Skip to content

RLC extension for resource collection#7602

Open
iamsanjaymalakar wants to merge 482 commits intotypetools:masterfrom
iamsanjaymalakar:rlc-for-collections
Open

RLC extension for resource collection#7602
iamsanjaymalakar wants to merge 482 commits intotypetools:masterfrom
iamsanjaymalakar:rlc-for-collections

Conversation

@iamsanjaymalakar
Copy link
Copy Markdown
Member

@iamsanjaymalakar iamsanjaymalakar commented Mar 31, 2026

This is the new PR for #7166

mernst and others added 30 commits June 26, 2025 10:24
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 11

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java (1)

824-834: ⚠️ Potential issue | 🟠 Major

Keep the synthetic-name counter instance-scoped.

Making uid static makes every artificial variable name depend on whatever CFGs were translated earlier, which adds unnecessary cross-analysis state and order-dependent names for iter, index, switchExpr, tempPostfix, etc. The previous instance field already guaranteed uniqueness within a single translation.

Suggested fix
-  protected static long uid = 0;
+  protected long uid = 0;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java`
around lines 824 - 834, The synthetic-name counter `uid` is currently static,
causing cross-analysis and order-dependent names; change its declaration in
class CFGTranslationPhaseOne from "protected static long uid = 0;" to an
instance field (e.g., "protected long uid = 0;") so that uniqueName(String
prefix) uses an instance-scoped counter and each translation gets its own
sequence; update any code that assumed a static field to use the instance field
if applicable.
♻️ Duplicate comments (24)
javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java (1)

1696-1699: ⚠️ Potential issue | 🔴 Critical

Guard argument access in getIdxForGetCall to avoid crash.

Line 1698 dereferences argument index 0 unconditionally. A zero-arg get() call will throw IndexOutOfBoundsException.

🐛 Proposed fix
 public static `@Nullable` ExpressionTree getIdxForGetCall(Tree tree) {
-  if ((tree instanceof MethodInvocationTree mit) && isNamedMethodCall("get", mit)) {
+  if ((tree instanceof MethodInvocationTree mit)
+      && isNamedMethodCall("get", mit)
+      && !mit.getArguments().isEmpty()) {
     return mit.getArguments().get(0);
   }
   return null;
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java` around
lines 1696 - 1699, getIdxForGetCall currently assumes the MethodInvocationTree
mit has at least one argument and calls mit.getArguments().get(0), which can
throw IndexOutOfBoundsException for zero-arg get() calls; update
getIdxForGetCall to first check the argument list (e.g.,
mit.getArguments().isEmpty() or size() > 0) before accessing index 0 and return
null when there are no arguments so zero-arg calls are safely handled.
checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.java (1)

177-221: ⚠️ Potential issue | 🟠 Major

Always recurse into nested declared type arguments.

replaceCollectionTypeVarsWithBottomIfTop still only descends into AnnotatedDeclaredType arguments when the outer type is a collection. That means wrappers like Optional<List<?>> or Holder<Iterator<?>> still skip the inner collection/iterator and leave its element type at @MustCallUnknown.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.java`
around lines 177 - 221, The current logic only enters the loop that calls
replaceCollectionTypeVarsWithBottomIfTop when
ResourceLeakUtils.isCollection(adt.getUnderlyingType()), so nested declared type
arguments inside non-collection wrappers (e.g., Optional<List<?>>) are never
traversed; update the traversal so that for any AnnotatedDeclaredType you always
recurse into its type arguments and call
replaceCollectionTypeVarsWithBottomIfTop on each declared type argument (use the
existing method replaceCollectionTypeVarsWithBottomIfTop and the same handling
used for typeArg.getKind() == TypeKind.DECLARED), not just when the outer adt is
a collection, ensuring inner collections/iterators have their element types
reset from `@MustCallUnknown` to `@MustCallBottom` when appropriate.
framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java (1)

1199-1214: ⚠️ Potential issue | 🟠 Major

Handle missing block input in getRegularStore.

flowResult.getInput(block) / analysis.getInput(block) can be absent for unreachable or unanalyzed blocks. This dereferences input unconditionally, so callers get an NPE instead of the nullable behavior used by the neighboring store accessors.

Suggested fix
-  public Store getRegularStore(Block block) {
+  public `@Nullable` Store getRegularStore(Block block) {
     TransferInput<Value, Store> input;
     if (!analysis.isRunning()) {
       input = flowResult.getInput(block);
     } else {
       input = analysis.getInput(block);
     }
-
-    return input.getRegularStore();
+    if (input == null) {
+      return null;
+    }
+    return input.getRegularStore();
   }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java`
around lines 1199 - 1214, The getRegularStore method dereferences the
TransferInput from flowResult.getInput(block) or analysis.getInput(block)
without null-check, causing NPE for unreachable/unanalyzed blocks; update
getRegularStore to check whether the selected TransferInput is null and return
null (matching neighboring store accessors' nullable behavior) when absent
instead of calling input.getRegularStore(), using the same selection logic
around analysis.isRunning(), and reference the symbols getRegularStore,
flowResult.getInput, analysis.getInput, and TransferInput in your change.
checker/src/main/java/org/checkerframework/checker/collectionownership/IndexedForDisposalLoopMatcher.java (1)

143-149: ⚠️ Potential issue | 🟠 Major

Use value equality for Name comparisons.

These Name checks rely on reference equality. That can silently miss valid matches or accept the wrong symbol when the compiler implementation does not reuse the same Name instance. Use .equals(...) / Objects.equals(...) throughout this matcher.

Based on learnings: In MustCallVisitor.java (Checker Framework), prefer using Name.equals(...) or Objects.equals(...) for com.sun.source.util.Name comparisons instead of ==/!=.

Also applies to: 190-225, 252-255

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/IndexedForDisposalLoopMatcher.java`
around lines 143 - 149, The code in IndexedForDisposalLoopMatcher uses reference
equality (== / !=) for com.sun.source.util.Name comparisons which is unsafe;
replace these with value equality using .equals(...) or Objects.equals(...) for
initVarName vs ((IdentifierTree) condition.getLeftOperand()).getName() and
initVarName vs ((IdentifierTree) inc.getExpression()).getName(), and update the
other similar checks in this class (the other Name comparisons around the later
matching logic) to use .equals/Objects.equals so name comparisons are by value
rather than reference.
checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java (1)

183-191: ⚠️ Potential issue | 🟠 Major

Lambda disposal loops still never reach discovery.

GenericAnnotatedTypeFactory.postCFGConstruction(...) is invoked for lambda CFGs too, but this guard only records disposal loops for METHOD. That means a disposal loop inside a lambda is never available to RLCCalledMethodsTransfer.initialStore(...), so collection obligations inside lambdas cannot be discharged through this path.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java`
around lines 183 - 191, The postCFGConstruction method only calls
ResourceLeakUtils.getCollectionOwnershipAnnotatedTypeFactory(this).discoverDisposalLoops(cfg)
when ast.getKind() == UnderlyingAST.Kind.METHOD, which skips lambda CFGs; update
the guard so disposal-loop discovery also runs for lambda CFGs (e.g.,
ast.getKind() == UnderlyingAST.Kind.METHOD || ast.getKind() ==
UnderlyingAST.Kind.LAMBDA) or remove the kind check entirely so
discoverDisposalLoops(cfg) runs for lambda CFGs as well, ensuring
RLCCalledMethodsTransfer.initialStore(...) sees disposal loops inside lambdas.
checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipTransfer.java (2)

313-313: 🧹 Nitpick | 🔵 Trivial

Prefer isEmpty() over size() == 0 for clarity.

Suggested improvement
-      boolean isDiamond = node.getTree().getTypeArguments().size() == 0;
+      boolean isDiamond = node.getTree().getTypeArguments().isEmpty();
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipTransfer.java`
at line 313, The expression checking for an empty type-arguments list uses
size() == 0 which is less clear; replace the check in
CollectionOwnershipTransfer where isDiamond is assigned (currently using
node.getTree().getTypeArguments().size() == 0) with the clearer isEmpty() call
on the type arguments collection (e.g.,
node.getTree().getTypeArguments().isEmpty()) so isDiamond uses isEmpty() instead
of size() == 0.

253-282: ⚠️ Potential issue | 🟠 Major

Guard nullable argElem before calling getKind().

TreeUtils.elementFromTree(arg.getTree()) can return null for temp vars, method results, and other non-element trees. Line 276 calls argElem.getKind() without checking for null, which will throw NullPointerException.

Proposed fix
       if (transferOwnership) {
-        if (argElem.getKind().isField()) {
+        if (argElem != null && argElem.getKind().isField()) {
           checker.reportError(
               arg.getTree(), "transfer.owningcollection.field.ownership", arg.getTree().toString());
         } else {
           replaceInStores(res, argJE, atypeFactory.NOTOWNINGCOLLECTION);
         }
       }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipTransfer.java`
around lines 253 - 282, Guard against a null Element returned by
TreeUtils.elementFromTree(arg.getTree()) before calling getKind(): in
CollectionOwnershipTransfer (around the block using argElem, transferOwnership,
and the calls to checker.reportError and replaceInStores) add a null-check so
that if argElem is null it is treated as "not a field" (i.e., call
replaceInStores(res, argJE, atypeFactory.NOTOWNINGCOLLECTION)) and only call
argElem.getKind().isField() when argElem != null; update the conditional that
currently does if (argElem.getKind().isField()) ... else ... to first test
argElem != null.
framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java (2)

79-80: ⚠️ Potential issue | 🟠 Major

Evict iterated-element facts after mutations.

The iteratedCollectionElements map is not cleared in mutation/invalidation paths. Methods like updateForMethodCall, removeConflicting(FieldAccess, ...), removeConflicting(ArrayAccess, ...), and removeConflicting(LocalVariable) do not invalidate entries in iteratedCollectionElements, which could allow stale facts to leak across iterations or after side-effecting operations.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java`
around lines 79 - 80, The map iteratedCollectionElements can retain stale facts
across mutations; update the mutation/invalidation paths to evict affected
entries by clearing or removing matching keys from iteratedCollectionElements:
in updateForMethodCall(...) and the removeConflicting(...) overloads
(removeConflicting(FieldAccess,...), removeConflicting(ArrayAccess,...),
removeConflicting(LocalVariable,...)) identify which IteratedCollectionElement
keys reference the mutated target and remove them (or clear the whole map when a
conservative global mutation occurs) so iteratedCollectionElements cannot leak
outdated facts after side-effecting operations.

841-848: ⚠️ Potential issue | 🟡 Minor

Loose OR matching logic should require both node and tree to match.

The condition on line 843 uses OR logic across four comparisons, returning the first IteratedCollectionElement where any condition is true. Since IteratedCollectionElement.equals() requires both tree and node to match, the lookup should also enforce both requirements to avoid returning incorrect entries.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java`
around lines 841 - 848, The lookup in getIteratedCollectionElement currently
returns an IteratedCollectionElement if any of four comparisons match; change
the logic to require both node and tree to match (mirroring
IteratedCollectionElement.equals). In method getIteratedCollectionElement,
iterate iteratedCollectionElements.keySet() and replace the compound condition
(ice.tree == tree || ice.node == node || ice.tree.equals(tree) ||
ice.node.equals(node)) with a check that both components match (e.g., (ice.tree
== tree || (ice.tree != null && ice.tree.equals(tree))) && (ice.node == node ||
(ice.node != null && ice.node.equals(node)))), then return ice only when both
match.
checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnalysis.java (1)

41-41: ⚠️ Potential issue | 🟡 Minor

Minor Javadoc formatting issue.

Line 41: missing space between "exact" and "{@link Throwable}".

Proposed fix
-   * <p>The policy follows the Resource Leak Checker configuration, but keeps exact{`@link` Throwable}
+   * <p>The policy follows the Resource Leak Checker configuration, but keeps exact {`@link` Throwable}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnalysis.java`
at line 41, In the Javadoc for class CollectionOwnershipAnalysis update the
comment that currently reads "exact{`@link` Throwable}" to insert a space so it
becomes "exact {`@link` Throwable}"; locate the Javadoc in
CollectionOwnershipAnalysis.java and adjust the spacing between "exact" and the
{`@link` Throwable} tag to fix the formatting.
checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipVisitor.java (1)

88-92: ⚠️ Potential issue | 🟠 Major

Fall back to declared receiver qualifier when flow store has no entry.

When getCoTypeAtTree returns null (no store entry), the method exits early without checking the declared type. This skips the @CreatesCollectionObligation enforcement for receivers whose ownership is only available from the declared type.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipVisitor.java`
around lines 88 - 92, When getCoTypeAtTree(receiverTree, storeBefore) returns
null, don't return immediately; instead obtain the declared/annotated receiver
type (via CollectionOwnershipAnnotatedTypeFactory.CollectionOwnershipType from
the annotated type factory using receiverTree or getAnnotatedType(receiverTree))
and assign it to receiverType so the subsequent `@CreatesCollectionObligation`
check runs even when the flow store lacks an entry; keep the existing null
guards and only return if both flow and declared lookups produce null.
checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java (5)

1023-1025: ⚠️ Potential issue | 🟠 Major

Don't compare Name instances with ==.

Different Name objects can have equal content, so this can miss the matching parameter slot and fail to propagate the declaration annotation to the use site.

Proposed fix
-            if (params.get(i).getSimpleName() == elt.getSimpleName()) {
+            if (params.get(i).getSimpleName().contentEquals(elt.getSimpleName())) {
               type.replaceAnnotation(paramTypes.get(i).getAnnotationInHierarchy(TOP));
               break;
             }

Based on learnings: In MustCallVisitor.java (Checker Framework), prefer using Name.equals(...) or Objects.equals(...) for com.sun.source.util.Name comparisons (instead of ==/!=). This should be the default unless the Interning Checker is explicitly used to guarantee reference equality.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java`
around lines 1023 - 1025, The code in CollectionOwnershipAnnotatedTypeFactory
compares Name objects using == (params.get(i).getSimpleName() ==
elt.getSimpleName()), which can miss matches; change the comparison to use
equals (e.g., params.get(i).getSimpleName().equals(elt.getSimpleName()) or
Objects.equals(...)) so the correct parameter slot from paramTypes is located
and the declaration annotation is propagated via
type.replaceAnnotation(paramTypes.get(i).getAnnotationInHierarchy(TOP)).

691-693: ⚠️ Potential issue | 🔴 Critical

Guard getAnnotatedType(tree) here and return null on unsupported trees.

This helper is called from the resource-leak analysis on arbitrary receiver trees. Without the same BugInCF guard used by isResourceCollection(Tree), unsupported tree shapes still crash analysis instead of yielding “no obligations”.

Based on learnings: In MustCallConsistencyAnalyzer.addObligationsForOwningCollectionReturn, treat a null result from CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(...) as “no obligations” and skip creating CollectionObligation(s); do not throw. This can occur for OwningCollection over non-resource element types.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java`
around lines 691 - 693, Guard the call to mcAtf.getAnnotatedType(tree) inside
CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent
by catching the same BugInCF/unsupported-tree condition used in
isResourceCollection(Tree) and return null when the tree shape is unsupported;
update getMustCallValuesOfResourceCollectionComponent to return null instead of
letting the exception bubble. Then update
MustCallConsistencyAnalyzer.addObligationsForOwningCollectionReturn to treat a
null return from
CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(...)
as “no obligations” (i.e., skip creating CollectionObligation instances for
OwningCollection element types when the annotated type cannot be determined) so
analysis does not crash on non-resource element types.

488-516: ⚠️ Potential issue | 🟡 Minor

Mirror the null guard from isOwningCollectionField(...).

isResourceCollectionField(...) and isOwningCollectionParameter(...) both dereference elt immediately. Callers already pass nullable elements elsewhere in this PR, so these helpers should return false instead of NPEing.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java`
around lines 488 - 516, Both isResourceCollectionField(Element elt) and
isOwningCollectionParameter(Element elt) assume elt is non-null and can NPE;
mirror the null guard used elsewhere by adding an initial null check in each
method (e.g., if (elt == null) return false;) before any elt dereferences. This
ensures calls to isResourceCollectionField and isOwningCollectionParameter
(which call elt.getKind(), elt.asType(), getAnnotatedType(elt), etc.) safely
return false for null elements without changing downstream logic that uses
getAnnotatedType, getCoType, or compares to
CollectionOwnershipType.OwningCollection.

609-620: ⚠️ Potential issue | 🟠 Major

Use stub-aware ownership checks for inserted arguments.

Element.getAnnotation(...) ignores annotations supplied via stub files. A stubbed @Owning or @NotOwning declaration will be misclassified here, which breaks collection-obligation transfer soundness. Use rlAtf.hasOwning(...) / rlAtf.hasNotOwning(...) instead.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java`
around lines 609 - 620, The code in CollectionOwnershipAnnotatedTypeFactory uses
Element.getAnnotation(...) on insertedElement (from insertedArgumentTree) which
ignores stub-file annotations; replace those checks with the stub-aware queries
rlAtf.hasNotOwning(insertedElement) and rlAtf.hasOwning(insertedElement) so the
logic that returns CollectionMutatorArgumentKind.DEFINITELY_NON_OWNING or
MAY_BE_OWNING remains the same but respects stub declarations; update both the
initial NotOwning check and the parameter Owning check to call
rlAtf.hasNotOwning(...) / rlAtf.hasOwning(...) on insertedElement instead of
getAnnotation(...).

657-678: ⚠️ Potential issue | 🟠 Major

Map<K,V> still falls through as “no component obligations”.

ResourceLeakUtils.isCollection(...) treats Map as collection-like, but both overloads only extract a component when there is exactly one type argument. Resource obligations on map values are silently skipped.

Also applies to: 706-722

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java`
around lines 657 - 678, getMustCallValuesOfResourceCollectionComponent currently
only extracts a single component type argument, so Map<K,V> (which has two type
arguments) falls through and value obligations are ignored; update the
extraction logic in getMustCallValuesOfResourceCollectionComponent to handle
both single-arg collection types and two-arg map-like types: if atm is an
AnnotatedDeclaredType with one type argument use that, and if it has two type
arguments (i.e., map-like) select the second argument as the component; then
pass that component to ResourceLeakUtils.getMcValues(componentType, mcAtf) as
before (refer to getMustCallValuesOfResourceCollectionComponent,
ResourceLeakUtils.isCollection, AnnotatedDeclaredType, and
ResourceLeakUtils.getMcValues).
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java (8)

3547-3560: ⚠️ Potential issue | 🔴 Critical

A null back-edge summary must invalidate the whole loop proof.

If one back edge returns null, skipping the intersection preserves methods learned from other back edges and can still certify an unverified disposal loop.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`
around lines 3547 - 3560, If any back-edge summary (calledMethodsAfterBlock
returned from analyzeTypeOfCollectionElement for the block within the loop) is
null, the entire loop proof must be invalidated instead of preserving prior
intersections; update the logic in the isLastBlockOfBody branch so that when
calledMethodsAfterBlock == null you set calledMethodsInLoop = null (and avoid
retaining previous values), using the existing variables calledMethodsInLoop,
calledMethodsAfterBlock, currentBlock, disposalLoop, obligations, and
loopUpdateBlock to locate the code and enforce that a single null summary clears
the accumulated calledMethodsInLoop.

2048-2053: ⚠️ Potential issue | 🟠 Major

Guard collection method-name lookup before calling .get(0).

Both diagnostics assume getMustCallValuesOfResourceCollectionComponent(...) returns a non-empty list. It can return null or an empty list, so these error paths can throw instead of reporting. Use the existing unknown-name fallback when no element method is available.

Based on learnings: When constructing diagnostics that reference an element method name, fall back to "Unknown" if the list is null/empty.

Also applies to: 2131-2135

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`
around lines 2048 - 2053, The call to
coAtf.getMustCallValuesOfResourceCollectionComponent(...) in
MustCallConsistencyAnalyzer is assumed non-empty before calling .get(0), which
can cause NPEs or IndexOutOfBounds; update both places that call
checker.reportError(...) (the one using that list and the similar block around
the other reportError) to first check for null or empty and extract a safe
method-name (use the existing "Unknown" fallback used elsewhere) before passing
it into checker.reportError; i.e., compute a safeMethodName = list != null &&
!list.isEmpty() ? list.get(0) : "Unknown" (or reuse the module's fallback
helper) and use that value in the reportError invocation.

3713-3715: ⚠️ Potential issue | 🟠 Major

Handle store.getValue(ice) == null before calling getCalledMethods(...).

getCalledMethods dereferences its argument, so this path can NPE on bottom / no-live-element states instead of treating the back-edge summary as unavailable.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`
around lines 3713 - 3715, The code calls getCalledMethods(cmValOfIce) without
checking that store.getValue(ice) returned a non-null AccumulationValue; guard
against null by assigning AccumulationValue cmValOfIce = store.getValue(ice) and
then checking cmValOfIce != null before invoking getCalledMethods(cmValOfIce)
(or treat a null as "no summary" and skip the back-edge handling). Update the
branch that uses cmValOfIce/calledMethods (referenced symbols: store.getValue,
ice, cmValOfIce, getCalledMethods) to return/continue when cmValOfIce is null so
getCalledMethods is never called on null.

965-974: ⚠️ Potential issue | 🟠 Major

Don't consume the inserted value after checkEnclosingMethodIsCreatesMustCallFor(...) fails.

On the error path, the code still removes the caller-side obligation for the inserted element. That makes the analysis behave as if ownership transferred anyway, which can hide a later leak on the original value.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`
around lines 965 - 974, The code currently always calls
consumeInsertedArgumentObligationIfSingleElementInsert(...), even when
checkEnclosingMethodIsCreatesMustCallFor(...) reports a failure; change the
control flow so the obligation is only consumed when the enclosing-method check
succeeds. Modify checkEnclosingMethodIsCreatesMustCallFor (or add a small
wrapper) to return a boolean success flag (e.g., true when no error reported) or
provide a way to detect failure, then call
consumeInsertedArgumentObligationIfSingleElementInsert(obligations, node) only
if that flag is true; update references in MustCallConsistencyAnalyzer around
receiverIsOwningField / receiverNode / enclosingMethodTree accordingly.

899-914: ⚠️ Potential issue | 🔴 Critical

Treat null component must-call values as “no obligations”.

getMustCallValuesOfResourceCollectionComponent(...) can legitimately return null for @OwningCollection over non-resource element types. Throwing BugInCF here turns a valid program into an analyzer crash.

Proposed fix
         List<String> mustCallValues =
             coAtf.getMustCallValuesOfResourceCollectionComponent(node.getTree());
-        if (mustCallValues == null) {
-          throw new BugInCF(
-              "List of MustCall values of component type is null for OwningCollection return value:"
-                  + " "
-                  + node);
-        }
-        if (!ResourceLeakUtils.isIterator(node.getType())) {
-          for (String mustCallMethod : mustCallValues) {
-            obligations.add(
-                new CollectionObligation(
-                    mustCallMethod, ImmutableSet.of(tmpVarAsResourceAlias), MethodExitKind.ALL));
-          }
+        if (mustCallValues == null || mustCallValues.isEmpty()
+            || ResourceLeakUtils.isIterator(node.getType())) {
+          return;
+        }
+        for (String mustCallMethod : mustCallValues) {
+          obligations.add(
+              new CollectionObligation(
+                  mustCallMethod, ImmutableSet.of(tmpVarAsResourceAlias), MethodExitKind.ALL));
         }

Based on learnings: In MustCallConsistencyAnalyzer.addObligationsForOwningCollectionReturn, treat a null result from CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(...) as “no obligations” and skip creating CollectionObligation(s); do not throw. This can occur for OwningCollection over non-resource element types.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`
around lines 899 - 914, In addObligationsForOwningCollectionReturn, stop
throwing a BugInCF when
coAtf.getMustCallValuesOfResourceCollectionComponent(node.getTree()) returns
null; instead treat null as “no obligations” and skip creating
CollectionObligation(s) for that case. Locate the check that assigns
mustCallValues from getMustCallValuesOfResourceCollectionComponent, and if
mustCallValues is null simply return (or continue) without creating
ResourceAlias tmpVarAsResourceAlias or adding obligations to the obligations
set; preserve existing behavior for non-null mustCallValues. Ensure references
include addObligationsForOwningCollectionReturn, cmAtf.getTempVarForNode,
coAtf.getStoreAfter / getCoType, and
coAtf.getMustCallValuesOfResourceCollectionComponent.

536-607: ⚠️ Potential issue | 🟠 Major

CollectionObligation still breaks the equals/hashCode contract.

mustCallMethod participates in equals, but hashCode is inherited from Obligation, and the field is mutable even though Obligation is treated as deeply immutable. Distinct obligations can collide or be corrupted in hash-based collections.

Proposed fix
   /** The method that must be called on all elements of the collection. */
-  public String mustCallMethod;
+  public final String mustCallMethod;
@@
     `@Override`
     public boolean equals(`@Nullable` Object obj) {
@@
       return super.equals(obj) && collectionObligation.mustCallMethod.equals(this.mustCallMethod);
     }
+
+    `@Override`
+    public int hashCode() {
+      return Objects.hash(super.hashCode(), mustCallMethod);
+    }
   }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`
around lines 536 - 607, CollectionObligation breaks equals/hashCode because
mustCallMethod is mutable and not included in hashCode; make mustCallMethod
final (change declaration to final and update constructors to set it) and
override hashCode in CollectionObligation to combine super.hashCode() with
mustCallMethod.hashCode() (e.g., 31*super.hashCode()+mustCallMethod.hashCode())
so equals and hashCode remain consistent for CollectionObligation instances;
keep getReplacement() behavior as-is.

2100-2105: ⚠️ Potential issue | 🔴 Critical

OwningCollectionBottom is the nullable-field case, not a BugInCF.

This branch is reached when the field currently holds null. Overwriting null with a new owning collection should be treated as a safe assignment instead of aborting analysis.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`
around lines 2100 - 2105, In MustCallConsistencyAnalyzer, the branch that checks
"lhsCoType == CollectionOwnershipType.OwningCollectionBottom" should not throw a
BugInCF because OwningCollectionBottom represents a nullable field being
overwritten; replace the throw with code that treats this as a safe assignment
(e.g., a no-op or appropriate continuation of analysis) and update the comment
to note that OwningCollectionBottom means the field may be null and can be
safely assigned; refer to the lhsCoType check in MustCallConsistencyAnalyzer and
remove the BugInCF construction for this case.

933-974: ⚠️ Potential issue | 🟠 Major

Model @CreatesCollectionObligation only on the normal edge, and enforce it on all later exits.

This helper is executed per successor edge, so it currently creates obligations on exceptional successors too, and CollectionObligation.fromTree(...) leaves whenToEnforce at NORMAL_RETURN. That can fabricate obligations in catch paths and under-enforce them after a successful insert.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`
around lines 933 - 974, The code currently creates collection obligations on
every successor edge; change addObligationsForCreatesCollectionObligationAnno so
it only creates obligations on the normal-successor (i.e., guard early with a
check for the normal/ordinary flow edge for the MethodInvocationNode) and when
constructing the obligation use the variant of CollectionObligation that sets
enforcement to "later exits" (or pass the appropriate whenToEnforce value
instead of leaving it at NORMAL_RETURN) so the obligation is enforced on all
subsequent exits; update the CollectionObligation.fromTree call to the
constructor/factory that accepts the whenToEnforce flag (or set the flag after
creation) and only call it when the node is on the normal path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipTransfer.java`:
- Around line 126-129: The code calls
disposalLoopCalledMethods.containsAll(requiredElementMethods) without checking
for null from atypeFactory.getMustCallValuesOfResourceCollectionComponent(...)
causing an NPE; update the logic in CollectionOwnershipTransfer to treat a null
requiredElementMethods as “no obligations” by first checking if
requiredElementMethods is null and, if so, skip creating any
CollectionObligation(s) (i.e., do not call containsAll or proceed with
obligation creation); specifically guard the requiredElementMethods variable
returned by getMustCallValuesOfResourceCollectionComponent(collectionExpression)
before uses like disposalLoopCalledMethods.containsAll(...) and short-circuit
the obligation-creation path when it is null.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipUtils.java`:
- Around line 119-145: The current getNameFromExpressionTree collapses
member-selected fields to only the terminal Name (mst.getIdentifier()), losing
distinction between different receiver expressions; change it to return a unique
identifier for the whole access instead of just the simple name—e.g., return the
Element obtained via TreeUtils.elementFromUse(mst) (or otherwise return a
representation that includes the receiver, not just mst.getIdentifier()) so
callers like IndexedForDisposalLoopMatcher can distinguish this.resources vs
other.resources; update the method signature/return type accordingly and replace
the branch that returns mst.getIdentifier() with returning the element (or a
qualified expression representation) and adjust callers to use the
element/qualified representation.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/EnhancedForDisposalLoopResolver.java`:
- Around line 143-144: The comparison in EnhancedForDisposalLoopResolver using
"iteratorVariableDeclaration.getName() == loopVariable.getName()" can give false
negatives; change it to use value equality (e.g.,
iteratorVariableDeclaration.getName().equals(loopVariable.getName()) or
Objects.equals(...)) when computing isAssignmentOfLoopVariable so Name instances
are compared correctly and null-safely; update the expression where
isAssignmentOfLoopVariable is set to use equals/Objects.equals on getName()
rather than ==.
- Around line 127-137: The code casts blocks to SingleSuccessorBlock without
checking types and can loop forever; update the logic around
methodInvocationNode.getBlock() and the successor traversal to first verify
instanceof SingleSuccessorBlock before casting, handle the case when
getSuccessor() returns null or a non-SingleSuccessorBlock (break/return or skip)
and guard against empty getNodes() to avoid infinite while loops; specifically
add instanceof checks before the casts to SingleSuccessorBlock and ensure the
loop that advances singleSuccessorBlock stops when successor is null or not the
expected type, and handle/return appropriately in the surrounding method
(EnhancedForDisposalLoopResolver) to preserve correctness of loopVariableNode
discovery.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/IndexedForDisposalLoopMatcher.java`:
- Around line 65-68: The matcher currently calls
TreeUtils.withoutParens(tree.getCondition()) without checking for a null
condition (ForLoopTree.getCondition() can be null for `for(;;)`), causing a NPE;
in IndexedForDisposalLoopMatcher, guard against a null condition by checking
tree.getCondition() (or the local before calling TreeUtils.withoutParens) and
immediately reject/return from the matcher when it's null so the code doesn't
attempt to strip parentheses from a null value.
- Around line 184-233: The scanner currently descends into nested scopes
(lambdas and local/anonymous classes) and can misattribute actions inside those
scopes to the outer loop; update the anonymous TreeScanner (scanner) to skip
nested scopes by overriding visitLambdaExpression(LambdaExpressionTree),
visitClass(ClassTree) and visitNewClass(NewClassTree) (or other nested-scope
nodes you see) to return null without calling super, so the scanner does not
traverse into lambda bodies or local/anonymous class bodies and only inspects
statements directly in the loop body; reference the existing scanner variable
and the methods visitUnary, visitAssignment, visitMethodInvocation to keep their
logic intact.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/WhileDisposalLoopMatcher.java`:
- Around line 659-665: In markWriteIfTargetsHeaderOrCollection, replace
reference-equality checks on javax.lang.model.element.Name with value-equality:
instead of comparing assigned == headerVar and assigned == collectionVarName,
call assigned.equals(headerVar) and (collectionVarName != null &&
assigned.equals(collectionVarName)); ensure you null-check assigned before
calling equals (or flip to headerVar.equals(assigned) if headerVar is non-null)
so the method correctly detects matching Names.
- Around line 751-752: Replace the reference-equality check in
WhileDisposalLoopMatcher where you compare Name objects: instead of "recv !=
null && recv == headerVar" use a value-equality check (e.g., "recv != null &&
recv.equals(headerVar)" or preferably "Objects.equals(recv, headerVar)") so Name
comparisons use equals() rather than ==; locate this via the
getNameFromExpressionTree(ms.getExpression()) call and the headerVar reference.

In
`@checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java`:
- Around line 451-457: getStoreAfterConditionalBlock currently asserts
transferInput non-null which fails when flowResult.getInput(block) is absent for
unreachable ConditionalBlock; replace the assert with an explicit null check:
call TransferInput<AccumulationValue,AccumulationStore> transferInput =
flowResult.getInput(block); if transferInput == null then return a safe empty
AccumulationStore (e.g. AccumulationStore.empty() or new AccumulationStore() —
create a static empty factory if needed); otherwise return
transferInput.getThenStore() or getElseStore() as before. Ensure you reference
getStoreAfterConditionalBlock, flowResult.getInput, TransferInput, and
AccumulationStore when making the change.

In
`@checker/src/test/java/org/checkerframework/checker/test/junit/ResourceLeakCollectionsTest.java`:
- Line 15: The test sets testDir to "resourceleak" but getTestDirs enumerates
"resourceleak-collections", causing mismatched test directories; update the
ResourceLeakCollectionsTest constructor (or the testDir variable) so it uses the
same directory string as getTestDirs (e.g., change testDir to
"resourceleak-collections"), or alternatively change getTestDirs to use the
constructor's testDir value—ensure the constructor, the testDir field, and the
getTestDirs method all reference the identical directory name.

In
`@dataflow/src/main/java/org/checkerframework/dataflow/expression/IteratedCollectionElement.java`:
- Around line 13-86: Add a toString() override on IteratedCollectionElement that
returns a concise, informative representation for debugging (e.g., include the
tree and/or node); implement it as a public `@Override` String toString() method
in the IteratedCollectionElement class that formats the output like
"IteratedCollectionElement(" + tree + ")" (or include node if desired) so
instances used as store keys or in logs are readable.

---

Outside diff comments:
In
`@dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java`:
- Around line 824-834: The synthetic-name counter `uid` is currently static,
causing cross-analysis and order-dependent names; change its declaration in
class CFGTranslationPhaseOne from "protected static long uid = 0;" to an
instance field (e.g., "protected long uid = 0;") so that uniqueName(String
prefix) uses an instance-scoped counter and each translation gets its own
sequence; update any code that assumed a static field to use the instance field
if applicable.

---

Duplicate comments:
In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnalysis.java`:
- Line 41: In the Javadoc for class CollectionOwnershipAnalysis update the
comment that currently reads "exact{`@link` Throwable}" to insert a space so it
becomes "exact {`@link` Throwable}"; locate the Javadoc in
CollectionOwnershipAnalysis.java and adjust the spacing between "exact" and the
{`@link` Throwable} tag to fix the formatting.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java`:
- Around line 1023-1025: The code in CollectionOwnershipAnnotatedTypeFactory
compares Name objects using == (params.get(i).getSimpleName() ==
elt.getSimpleName()), which can miss matches; change the comparison to use
equals (e.g., params.get(i).getSimpleName().equals(elt.getSimpleName()) or
Objects.equals(...)) so the correct parameter slot from paramTypes is located
and the declaration annotation is propagated via
type.replaceAnnotation(paramTypes.get(i).getAnnotationInHierarchy(TOP)).
- Around line 691-693: Guard the call to mcAtf.getAnnotatedType(tree) inside
CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent
by catching the same BugInCF/unsupported-tree condition used in
isResourceCollection(Tree) and return null when the tree shape is unsupported;
update getMustCallValuesOfResourceCollectionComponent to return null instead of
letting the exception bubble. Then update
MustCallConsistencyAnalyzer.addObligationsForOwningCollectionReturn to treat a
null return from
CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(...)
as “no obligations” (i.e., skip creating CollectionObligation instances for
OwningCollection element types when the annotated type cannot be determined) so
analysis does not crash on non-resource element types.
- Around line 488-516: Both isResourceCollectionField(Element elt) and
isOwningCollectionParameter(Element elt) assume elt is non-null and can NPE;
mirror the null guard used elsewhere by adding an initial null check in each
method (e.g., if (elt == null) return false;) before any elt dereferences. This
ensures calls to isResourceCollectionField and isOwningCollectionParameter
(which call elt.getKind(), elt.asType(), getAnnotatedType(elt), etc.) safely
return false for null elements without changing downstream logic that uses
getAnnotatedType, getCoType, or compares to
CollectionOwnershipType.OwningCollection.
- Around line 609-620: The code in CollectionOwnershipAnnotatedTypeFactory uses
Element.getAnnotation(...) on insertedElement (from insertedArgumentTree) which
ignores stub-file annotations; replace those checks with the stub-aware queries
rlAtf.hasNotOwning(insertedElement) and rlAtf.hasOwning(insertedElement) so the
logic that returns CollectionMutatorArgumentKind.DEFINITELY_NON_OWNING or
MAY_BE_OWNING remains the same but respects stub declarations; update both the
initial NotOwning check and the parameter Owning check to call
rlAtf.hasNotOwning(...) / rlAtf.hasOwning(...) on insertedElement instead of
getAnnotation(...).
- Around line 657-678: getMustCallValuesOfResourceCollectionComponent currently
only extracts a single component type argument, so Map<K,V> (which has two type
arguments) falls through and value obligations are ignored; update the
extraction logic in getMustCallValuesOfResourceCollectionComponent to handle
both single-arg collection types and two-arg map-like types: if atm is an
AnnotatedDeclaredType with one type argument use that, and if it has two type
arguments (i.e., map-like) select the second argument as the component; then
pass that component to ResourceLeakUtils.getMcValues(componentType, mcAtf) as
before (refer to getMustCallValuesOfResourceCollectionComponent,
ResourceLeakUtils.isCollection, AnnotatedDeclaredType, and
ResourceLeakUtils.getMcValues).

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipTransfer.java`:
- Line 313: The expression checking for an empty type-arguments list uses size()
== 0 which is less clear; replace the check in CollectionOwnershipTransfer where
isDiamond is assigned (currently using node.getTree().getTypeArguments().size()
== 0) with the clearer isEmpty() call on the type arguments collection (e.g.,
node.getTree().getTypeArguments().isEmpty()) so isDiamond uses isEmpty() instead
of size() == 0.
- Around line 253-282: Guard against a null Element returned by
TreeUtils.elementFromTree(arg.getTree()) before calling getKind(): in
CollectionOwnershipTransfer (around the block using argElem, transferOwnership,
and the calls to checker.reportError and replaceInStores) add a null-check so
that if argElem is null it is treated as "not a field" (i.e., call
replaceInStores(res, argJE, atypeFactory.NOTOWNINGCOLLECTION)) and only call
argElem.getKind().isField() when argElem != null; update the conditional that
currently does if (argElem.getKind().isField()) ... else ... to first test
argElem != null.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipVisitor.java`:
- Around line 88-92: When getCoTypeAtTree(receiverTree, storeBefore) returns
null, don't return immediately; instead obtain the declared/annotated receiver
type (via CollectionOwnershipAnnotatedTypeFactory.CollectionOwnershipType from
the annotated type factory using receiverTree or getAnnotatedType(receiverTree))
and assign it to receiverType so the subsequent `@CreatesCollectionObligation`
check runs even when the flow store lacks an entry; keep the existing null
guards and only return if both flow and declared lookups produce null.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/IndexedForDisposalLoopMatcher.java`:
- Around line 143-149: The code in IndexedForDisposalLoopMatcher uses reference
equality (== / !=) for com.sun.source.util.Name comparisons which is unsafe;
replace these with value equality using .equals(...) or Objects.equals(...) for
initVarName vs ((IdentifierTree) condition.getLeftOperand()).getName() and
initVarName vs ((IdentifierTree) inc.getExpression()).getName(), and update the
other similar checks in this class (the other Name comparisons around the later
matching logic) to use .equals/Objects.equals so name comparisons are by value
rather than reference.

In
`@checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.java`:
- Around line 177-221: The current logic only enters the loop that calls
replaceCollectionTypeVarsWithBottomIfTop when
ResourceLeakUtils.isCollection(adt.getUnderlyingType()), so nested declared type
arguments inside non-collection wrappers (e.g., Optional<List<?>>) are never
traversed; update the traversal so that for any AnnotatedDeclaredType you always
recurse into its type arguments and call
replaceCollectionTypeVarsWithBottomIfTop on each declared type argument (use the
existing method replaceCollectionTypeVarsWithBottomIfTop and the same handling
used for typeArg.getKind() == TypeKind.DECLARED), not just when the outer adt is
a collection, ensuring inner collections/iterators have their element types
reset from `@MustCallUnknown` to `@MustCallBottom` when appropriate.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`:
- Around line 3547-3560: If any back-edge summary (calledMethodsAfterBlock
returned from analyzeTypeOfCollectionElement for the block within the loop) is
null, the entire loop proof must be invalidated instead of preserving prior
intersections; update the logic in the isLastBlockOfBody branch so that when
calledMethodsAfterBlock == null you set calledMethodsInLoop = null (and avoid
retaining previous values), using the existing variables calledMethodsInLoop,
calledMethodsAfterBlock, currentBlock, disposalLoop, obligations, and
loopUpdateBlock to locate the code and enforce that a single null summary clears
the accumulated calledMethodsInLoop.
- Around line 2048-2053: The call to
coAtf.getMustCallValuesOfResourceCollectionComponent(...) in
MustCallConsistencyAnalyzer is assumed non-empty before calling .get(0), which
can cause NPEs or IndexOutOfBounds; update both places that call
checker.reportError(...) (the one using that list and the similar block around
the other reportError) to first check for null or empty and extract a safe
method-name (use the existing "Unknown" fallback used elsewhere) before passing
it into checker.reportError; i.e., compute a safeMethodName = list != null &&
!list.isEmpty() ? list.get(0) : "Unknown" (or reuse the module's fallback
helper) and use that value in the reportError invocation.
- Around line 3713-3715: The code calls getCalledMethods(cmValOfIce) without
checking that store.getValue(ice) returned a non-null AccumulationValue; guard
against null by assigning AccumulationValue cmValOfIce = store.getValue(ice) and
then checking cmValOfIce != null before invoking getCalledMethods(cmValOfIce)
(or treat a null as "no summary" and skip the back-edge handling). Update the
branch that uses cmValOfIce/calledMethods (referenced symbols: store.getValue,
ice, cmValOfIce, getCalledMethods) to return/continue when cmValOfIce is null so
getCalledMethods is never called on null.
- Around line 965-974: The code currently always calls
consumeInsertedArgumentObligationIfSingleElementInsert(...), even when
checkEnclosingMethodIsCreatesMustCallFor(...) reports a failure; change the
control flow so the obligation is only consumed when the enclosing-method check
succeeds. Modify checkEnclosingMethodIsCreatesMustCallFor (or add a small
wrapper) to return a boolean success flag (e.g., true when no error reported) or
provide a way to detect failure, then call
consumeInsertedArgumentObligationIfSingleElementInsert(obligations, node) only
if that flag is true; update references in MustCallConsistencyAnalyzer around
receiverIsOwningField / receiverNode / enclosingMethodTree accordingly.
- Around line 899-914: In addObligationsForOwningCollectionReturn, stop throwing
a BugInCF when
coAtf.getMustCallValuesOfResourceCollectionComponent(node.getTree()) returns
null; instead treat null as “no obligations” and skip creating
CollectionObligation(s) for that case. Locate the check that assigns
mustCallValues from getMustCallValuesOfResourceCollectionComponent, and if
mustCallValues is null simply return (or continue) without creating
ResourceAlias tmpVarAsResourceAlias or adding obligations to the obligations
set; preserve existing behavior for non-null mustCallValues. Ensure references
include addObligationsForOwningCollectionReturn, cmAtf.getTempVarForNode,
coAtf.getStoreAfter / getCoType, and
coAtf.getMustCallValuesOfResourceCollectionComponent.
- Around line 536-607: CollectionObligation breaks equals/hashCode because
mustCallMethod is mutable and not included in hashCode; make mustCallMethod
final (change declaration to final and update constructors to set it) and
override hashCode in CollectionObligation to combine super.hashCode() with
mustCallMethod.hashCode() (e.g., 31*super.hashCode()+mustCallMethod.hashCode())
so equals and hashCode remain consistent for CollectionObligation instances;
keep getReplacement() behavior as-is.
- Around line 2100-2105: In MustCallConsistencyAnalyzer, the branch that checks
"lhsCoType == CollectionOwnershipType.OwningCollectionBottom" should not throw a
BugInCF because OwningCollectionBottom represents a nullable field being
overwritten; replace the throw with code that treats this as a safe assignment
(e.g., a no-op or appropriate continuation of analysis) and update the comment
to note that OwningCollectionBottom means the field may be null and can be
safely assigned; refer to the lhsCoType check in MustCallConsistencyAnalyzer and
remove the BugInCF construction for this case.
- Around line 933-974: The code currently creates collection obligations on
every successor edge; change addObligationsForCreatesCollectionObligationAnno so
it only creates obligations on the normal-successor (i.e., guard early with a
check for the normal/ordinary flow edge for the MethodInvocationNode) and when
constructing the obligation use the variant of CollectionObligation that sets
enforcement to "later exits" (or pass the appropriate whenToEnforce value
instead of leaving it at NORMAL_RETURN) so the obligation is enforced on all
subsequent exits; update the CollectionObligation.fromTree call to the
constructor/factory that accepts the whenToEnforce flag (or set the flag after
creation) and only call it when the node is on the normal path.

In
`@checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java`:
- Around line 183-191: The postCFGConstruction method only calls
ResourceLeakUtils.getCollectionOwnershipAnnotatedTypeFactory(this).discoverDisposalLoops(cfg)
when ast.getKind() == UnderlyingAST.Kind.METHOD, which skips lambda CFGs; update
the guard so disposal-loop discovery also runs for lambda CFGs (e.g.,
ast.getKind() == UnderlyingAST.Kind.METHOD || ast.getKind() ==
UnderlyingAST.Kind.LAMBDA) or remove the kind check entirely so
discoverDisposalLoops(cfg) runs for lambda CFGs as well, ensuring
RLCCalledMethodsTransfer.initialStore(...) sees disposal loops inside lambdas.

In
`@framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java`:
- Around line 79-80: The map iteratedCollectionElements can retain stale facts
across mutations; update the mutation/invalidation paths to evict affected
entries by clearing or removing matching keys from iteratedCollectionElements:
in updateForMethodCall(...) and the removeConflicting(...) overloads
(removeConflicting(FieldAccess,...), removeConflicting(ArrayAccess,...),
removeConflicting(LocalVariable,...)) identify which IteratedCollectionElement
keys reference the mutated target and remove them (or clear the whole map when a
conservative global mutation occurs) so iteratedCollectionElements cannot leak
outdated facts after side-effecting operations.
- Around line 841-848: The lookup in getIteratedCollectionElement currently
returns an IteratedCollectionElement if any of four comparisons match; change
the logic to require both node and tree to match (mirroring
IteratedCollectionElement.equals). In method getIteratedCollectionElement,
iterate iteratedCollectionElements.keySet() and replace the compound condition
(ice.tree == tree || ice.node == node || ice.tree.equals(tree) ||
ice.node.equals(node)) with a check that both components match (e.g., (ice.tree
== tree || (ice.tree != null && ice.tree.equals(tree))) && (ice.node == node ||
(ice.node != null && ice.node.equals(node)))), then return ice only when both
match.

In
`@framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java`:
- Around line 1199-1214: The getRegularStore method dereferences the
TransferInput from flowResult.getInput(block) or analysis.getInput(block)
without null-check, causing NPE for unreachable/unanalyzed blocks; update
getRegularStore to check whether the selected TransferInput is null and return
null (matching neighboring store accessors' nullable behavior) when absent
instead of calling input.getRegularStore(), using the same selection logic
around analysis.isRunning(), and reference the symbols getRegularStore,
flowResult.getInput, analysis.getInput, and TransferInput in your change.

In `@javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java`:
- Around line 1696-1699: getIdxForGetCall currently assumes the
MethodInvocationTree mit has at least one argument and calls
mit.getArguments().get(0), which can throw IndexOutOfBoundsException for
zero-arg get() calls; update getIdxForGetCall to first check the argument list
(e.g., mit.getArguments().isEmpty() or size() > 0) before accessing index 0 and
return null when there are no arguments so zero-arg calls are safely handled.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 49f0d195-b2db-4da3-8ef3-7eceb187f82f

📥 Commits

Reviewing files that changed from the base of the PR and between ea35dc8 and b75ec8f.

📒 Files selected for processing (20)
  • checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnalysis.java
  • checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java
  • checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipTransfer.java
  • checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipUtils.java
  • checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipVisitor.java
  • checker/src/main/java/org/checkerframework/checker/collectionownership/DisposalLoop.java
  • checker/src/main/java/org/checkerframework/checker/collectionownership/DisposalLoopScanner.java
  • checker/src/main/java/org/checkerframework/checker/collectionownership/EnhancedForDisposalLoopResolver.java
  • checker/src/main/java/org/checkerframework/checker/collectionownership/IndexedForDisposalLoopMatcher.java
  • checker/src/main/java/org/checkerframework/checker/collectionownership/WhileDisposalLoopMatcher.java
  • checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.java
  • checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
  • checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java
  • checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsTransfer.java
  • checker/src/test/java/org/checkerframework/checker/test/junit/ResourceLeakCollectionsTest.java
  • dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java
  • dataflow/src/main/java/org/checkerframework/dataflow/expression/IteratedCollectionElement.java
  • framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java
  • framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java
  • javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java

Comment on lines +119 to +145
static Name getNameFromExpressionTree(ExpressionTree expr) {
if (expr == null) {
return null;
}
switch (expr.getKind()) {
case IDENTIFIER -> {
return ((com.sun.source.tree.IdentifierTree) expr).getName();
}
case MEMBER_SELECT -> {
MemberSelectTree mst = (MemberSelectTree) expr;
Element elt = TreeUtils.elementFromUse(mst);
if (elt.getKind() == ElementKind.FIELD) {
return mst.getIdentifier();
} else if (elt.getKind() == ElementKind.METHOD) {
return getNameFromExpressionTree(mst.getExpression());
} else {
return null;
}
}
case METHOD_INVOCATION -> {
return getNameFromExpressionTree(((MethodInvocationTree) expr).getMethodSelect());
}
default -> {
return null;
}
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Don't collapse member-selected collections to a bare Name.

For field/member accesses this returns only the terminal identifier. That makes this.resources, other.resources, and any other same-named field indistinguishable to callers like IndexedForDisposalLoopMatcher, which can then bind a loop header/body check to the wrong collection instance. Compare full expressions or elements here, not just the simple name.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipUtils.java`
around lines 119 - 145, The current getNameFromExpressionTree collapses
member-selected fields to only the terminal Name (mst.getIdentifier()), losing
distinction between different receiver expressions; change it to return a unique
identifier for the whole access instead of just the simple name—e.g., return the
Element obtained via TreeUtils.elementFromUse(mst) (or otherwise return a
representation that includes the receiver, not just mst.getIdentifier()) so
callers like IndexedForDisposalLoopMatcher can distinguish this.resources vs
other.resources; update the method signature/return type accordingly and replace
the branch that returns mst.getIdentifier() with returning the element (or a
qualified expression representation) and adjust callers to use the
element/qualified representation.

Comment on lines +451 to +457
public AccumulationStore getStoreAfterConditionalBlock(ConditionalBlock block, boolean then) {
TransferInput<AccumulationValue, AccumulationStore> transferInput = flowResult.getInput(block);
assert transferInput != null : "@AssumeAssertion(nullness): transferInput should be non-null";
if (then) {
return transferInput.getThenStore();
}
return transferInput.getElseStore();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Don't rely on assert for missing conditional-block input.

flowResult.getInput(block) can be absent for unreachable blocks. With assertions disabled this falls through to transferInput.getThenStore() / getElseStore() and crashes with an NPE.

Suggested fix
-  public AccumulationStore getStoreAfterConditionalBlock(ConditionalBlock block, boolean then) {
+  public `@Nullable` AccumulationStore getStoreAfterConditionalBlock(
+      ConditionalBlock block, boolean then) {
     TransferInput<AccumulationValue, AccumulationStore> transferInput = flowResult.getInput(block);
-    assert transferInput != null : "@AssumeAssertion(nullness): transferInput should be non-null";
+    if (transferInput == null) {
+      return null;
+    }
     if (then) {
       return transferInput.getThenStore();
     }
     return transferInput.getElseStore();
   }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java`
around lines 451 - 457, getStoreAfterConditionalBlock currently asserts
transferInput non-null which fails when flowResult.getInput(block) is absent for
unreachable ConditionalBlock; replace the assert with an explicit null check:
call TransferInput<AccumulationValue,AccumulationStore> transferInput =
flowResult.getInput(block); if transferInput == null then return a safe empty
AccumulationStore (e.g. AccumulationStore.empty() or new AccumulationStore() —
create a static empty factory if needed); otherwise return
transferInput.getThenStore() or getElseStore() as before. Ensure you reference
getStoreAfterConditionalBlock, flowResult.getInput, TransferInput, and
AccumulationStore when making the change.

super(
testFiles,
ResourceLeakChecker.class,
"resourceleak",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Use the same test directory in constructor and getTestDirs.

Line 15 sets testDir to "resourceleak", but Line 23 discovers files under "resourceleak-collections". This can cause the harness to resolve configuration/diagnostics relative to a different directory than the one being enumerated.

Proposed fix
-        "resourceleak",
+        "resourceleak-collections",
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
"resourceleak",
"resourceleak-collections",
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@checker/src/test/java/org/checkerframework/checker/test/junit/ResourceLeakCollectionsTest.java`
at line 15, The test sets testDir to "resourceleak" but getTestDirs enumerates
"resourceleak-collections", causing mismatched test directories; update the
ResourceLeakCollectionsTest constructor (or the testDir variable) so it uses the
same directory string as getTestDirs (e.g., change testDir to
"resourceleak-collections"), or alternatively change getTestDirs to use the
constructor's testDir value—ensure the constructor, the testDir field, and the
getTestDirs method all reference the identical directory name.

Comment on lines +13 to +86
public class IteratedCollectionElement extends JavaExpression {
/** The CFG node for this collection element. */
public final Node node;

/** The AST node for this collection element. */
public final Tree tree;

/**
* Creates a new IteratedCollectionElement.
*
* @param node CFG node
* @param tree an AST tree
*/
public IteratedCollectionElement(Node node, Tree tree) {
super(node.getType());
this.node = node;
this.tree = tree;
}

@Override
public boolean equals(@Nullable Object obj) {
if (this == obj) {
return true;
}
if (!(obj instanceof IteratedCollectionElement other)) {
return false;
}
return this.tree.equals(other.tree) && this.node.equals(other.node);
}

@Override
public int hashCode() {
return Objects.hash(tree, node);
}

@SuppressWarnings("unchecked") // generic cast
@Override
public <T extends JavaExpression> @Nullable T containedOfClass(Class<T> clazz) {
return getClass() == clazz ? (T) this : null;
}

@Override
public boolean isDeterministic(AnnotationProvider provider) {
return true;
}

@Override
public boolean syntacticEquals(JavaExpression je) {
if (!(je instanceof IteratedCollectionElement other)) {
return false;
}
return this.equals(other);
}

@Override
public boolean containsSyntacticEqualJavaExpression(JavaExpression other) {
return syntacticEquals(other);
}

@Override
public boolean isAssignableByOtherCode() {
return false;
}

@Override
public boolean isModifiableByOtherCode() {
return false;
}

@Override
public <R, P> R accept(JavaExpressionVisitor<R, P> visitor, P p) {
return visitor.visitIteratedCollectionElement(this, p);
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider adding a toString() method for debugging.

The class lacks a toString() override. Since IteratedCollectionElement is used as a store key and may appear in error messages or debug output, a meaningful string representation would help with troubleshooting.

Suggested implementation
`@Override`
public String toString() {
  return "IteratedCollectionElement(" + tree + ")";
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@dataflow/src/main/java/org/checkerframework/dataflow/expression/IteratedCollectionElement.java`
around lines 13 - 86, Add a toString() override on IteratedCollectionElement
that returns a concise, informative representation for debugging (e.g., include
the tree and/or node); implement it as a public `@Override` String toString()
method in the IteratedCollectionElement class that formats the output like
"IteratedCollectionElement(" + tree + ")" (or include node if desired) so
instances used as store keys or in logs are readable.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

♻️ Duplicate comments (3)
checker/src/main/java/org/checkerframework/checker/collectionownership/WhileDisposalLoopMatcher.java (1)

655-660: ⚠️ Potential issue | 🟡 Minor

Use value equality for Name comparisons.

Line 658, Line 659, and Line 748 compare Name objects with ==, which can mis-match equivalent names and break loop matching/invalid-write detection.

Proposed fix
           private void markWriteIfTargetsHeaderOrCollection(ExpressionTree lhs) {
             Name assigned = CollectionOwnershipUtils.getNameFromExpressionTree(lhs);
             if (assigned != null) {
-              if (assigned == headerVar) illegal.set(true);
-              if (collectionVarName != null && assigned == collectionVarName) illegal.set(true);
+              if (assigned.equals(headerVar)) illegal.set(true);
+              if (collectionVarName != null && assigned.equals(collectionVarName)) illegal.set(true);
             }
           }
@@
     Name recv = CollectionOwnershipUtils.getNameFromExpressionTree(ms.getExpression());
-    return recv != null && recv == headerVar;
+    return recv != null && recv.equals(headerVar);
   }
#!/bin/bash
# Verify there are no remaining reference-equality checks on Name for these comparisons.
rg -n --type=java -C2 '\b(assigned|recv)\s*==\s*(headerVar|collectionVarName)\b' \
  checker/src/main/java/org/checkerframework/checker/collectionownership/WhileDisposalLoopMatcher.java
# Expected: no matches

Based on learnings: prefer using Name.equals(...) or Objects.equals(...) for Name comparisons instead of ==/!=.

Also applies to: 747-748

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/WhileDisposalLoopMatcher.java`
around lines 655 - 660, Replace reference-equality checks on Name with
value-equality: in WhileDisposalLoopMatcher (method
markWriteIfTargetsHeaderOrCollection) change comparisons like assigned ==
headerVar and assigned == collectionVarName to use Name.equals(...) or
Objects.equals(assigned, headerVar) / Objects.equals(assigned,
collectionVarName); likewise update the recv == headerVar / recv ==
collectionVarName checks around the recv handling (lines referenced near
747-748) to use equals/Objects.equals so equivalent Name instances compare
correctly.
checker/src/main/java/org/checkerframework/checker/collectionownership/IndexedForDisposalLoopMatcher.java (2)

64-65: ⚠️ Potential issue | 🟠 Major

Still guard the loop condition before stripping parens.

ForLoopTree.getCondition() can be null, so TreeUtils.withoutParens(...) will still throw on for (;;) before the matcher can reject the loop.

Suggested fix
-    ExpressionTree condition = TreeUtils.withoutParens(tree.getCondition());
+    ExpressionTree rawCondition = tree.getCondition();
+    if (rawCondition == null) {
+      return null;
+    }
+    ExpressionTree condition = TreeUtils.withoutParens(rawCondition);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/IndexedForDisposalLoopMatcher.java`
around lines 64 - 65, The code calls
TreeUtils.withoutParens(tree.getCondition()) without guarding against
tree.getCondition() being null, which will throw for an empty-condition
for-loop; modify the logic in IndexedForDisposalLoopMatcher (the method handling
the ForLoopTree) to first check tree.getCondition() != null (and only then call
TreeUtils.withoutParens), returning/rejecting the loop early if the condition is
null so the matcher doesn't throw.

181-226: ⚠️ Potential issue | 🟠 Major

Skip nested scopes in the body scan.

This scanner still descends into lambdas and local/anonymous classes, so a write or collection.get(i) inside an unrelated nested scope can incorrectly certify or invalidate the outer loop.

Suggested fix
 import com.sun.source.tree.AssignmentTree;
 import com.sun.source.tree.BinaryTree;
 import com.sun.source.tree.CompoundAssignmentTree;
+import com.sun.source.tree.ClassTree;
 import com.sun.source.tree.ExpressionStatementTree;
 import com.sun.source.tree.ExpressionTree;
 import com.sun.source.tree.ForLoopTree;
 import com.sun.source.tree.IdentifierTree;
 import com.sun.source.tree.LiteralTree;
+import com.sun.source.tree.LambdaExpressionTree;
 import com.sun.source.tree.MemberSelectTree;
 import com.sun.source.tree.MethodInvocationTree;
+import com.sun.source.tree.NewClassTree;
 import com.sun.source.tree.StatementTree;
 import com.sun.source.tree.Tree;
 import com.sun.source.tree.UnaryTree;
 import com.sun.source.tree.VariableTree;
@@
         new TreeScanner<Void, Void>() {
+          `@Override`
+          public Void visitLambdaExpression(LambdaExpressionTree tree, Void p) {
+            return null;
+          }
+
+          `@Override`
+          public Void visitClass(ClassTree tree, Void p) {
+            return null;
+          }
+
+          `@Override`
+          public Void visitNewClass(NewClassTree tree, Void p) {
+            return null;
+          }
+
           `@Override`
           public Void visitUnary(UnaryTree tree, Void p) {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/IndexedForDisposalLoopMatcher.java`
around lines 181 - 226, The TreeScanner currently descends into nested scopes
(lambdas and local/anonymous classes) and thus can see writes or
collection.get(i) in unrelated scopes; fix by overriding the scope-entry visit
methods on the anonymous TreeScanner (the scanner variable in
IndexedForDisposalLoopMatcher) to stop traversal into nested scopes — add
overrides for visitLambdaExpression(LambdaExpressionTree),
visitNewClass(NewClassTree) and visitClass(ClassTree) that simply return null
(do not call super) so the scanner skips descending into those nested scopes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/EnhancedForDisposalLoopResolver.java`:
- Around line 165-169: The current backward scan accepts the first hasNext() it
finds (in the block where node instanceof MethodInvocationNode) without ensuring
that the found MethodInvocationTree actually belongs to the target enhanced-for
condition `tree`; update the check so after obtaining methodInvocationTree and
confirming TreeUtils.isHasNextCall(methodInvocationTree) you also verify the
found methodInvocationTree is the same call (or is contained in the same parent
tree) as the target `tree` before setting isLoopCondition/returning — i.e.,
compare methodInvocationTree to `tree` (or its relevant parent) to ensure the
hasNext() belongs to the target enhanced-for loop.
- Around line 172-184: The code currently throws BugInCF when the CFG shape
doesn't match (in the checks on
blockContainingLoopCondition.getSuccessors().size() and the instanceof check for
ConditionalBlock), but we should instead reject the candidate by returning null;
change both throw new BugInCF(...) occurrences in
EnhancedForDisposalLoopResolver (the checks using blockContainingLoopCondition,
maybeConditionalBlock and the instanceof ConditionalBlock conditionalBlock) to
return null so the resolver gracefully skips non-matching CFG shapes rather than
crashing.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/IndexedForDisposalLoopMatcher.java`:
- Around line 78-95: The code currently casts loopConditionBlock to
SingleSuccessorBlock and its successor to ConditionalBlock without checks
causing potential ClassCastException; update IndexedForDisposalLoopMatcher to
verify types (use instanceof) before casting: ensure loopConditionBlock is a
SingleSuccessorBlock, get its successor and ensure that successor is a
ConditionalBlock, and only then obtain loopBodyEntryBlock and construct the
DisposalLoop; if any instanceof checks fail (or
loopUpdateBlock/nodeForCollectionElt are null) return null—mirror the pattern
used in WhileDisposalLoopMatcher.findConditionalSuccessor() for safe CFG
handling.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/WhileDisposalLoopMatcher.java`:
- Around line 826-829: In WhileDisposalLoopMatcher, the code selects the wrong
block for loop updates by storing backEdge.targetBlock (the loop header) into
bestLoopUpdateBlock; change the assignment to use the back-edge source
(backEdge.sourceBlock) so bestLoopUpdateBlock points to the loop tail/update
block, and ensure the variable types and any subsequent uses of
bestLoopUpdateBlock still match this source-block semantics.

---

Duplicate comments:
In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/IndexedForDisposalLoopMatcher.java`:
- Around line 64-65: The code calls TreeUtils.withoutParens(tree.getCondition())
without guarding against tree.getCondition() being null, which will throw for an
empty-condition for-loop; modify the logic in IndexedForDisposalLoopMatcher (the
method handling the ForLoopTree) to first check tree.getCondition() != null (and
only then call TreeUtils.withoutParens), returning/rejecting the loop early if
the condition is null so the matcher doesn't throw.
- Around line 181-226: The TreeScanner currently descends into nested scopes
(lambdas and local/anonymous classes) and thus can see writes or
collection.get(i) in unrelated scopes; fix by overriding the scope-entry visit
methods on the anonymous TreeScanner (the scanner variable in
IndexedForDisposalLoopMatcher) to stop traversal into nested scopes — add
overrides for visitLambdaExpression(LambdaExpressionTree),
visitNewClass(NewClassTree) and visitClass(ClassTree) that simply return null
(do not call super) so the scanner skips descending into those nested scopes.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/WhileDisposalLoopMatcher.java`:
- Around line 655-660: Replace reference-equality checks on Name with
value-equality: in WhileDisposalLoopMatcher (method
markWriteIfTargetsHeaderOrCollection) change comparisons like assigned ==
headerVar and assigned == collectionVarName to use Name.equals(...) or
Objects.equals(assigned, headerVar) / Objects.equals(assigned,
collectionVarName); likewise update the recv == headerVar / recv ==
collectionVarName checks around the recv handling (lines referenced near
747-748) to use equals/Objects.equals so equivalent Name instances compare
correctly.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: fb66b8c1-1600-420b-887e-94d0e962bf1b

📥 Commits

Reviewing files that changed from the base of the PR and between b75ec8f and a4c20aa.

📒 Files selected for processing (3)
  • checker/src/main/java/org/checkerframework/checker/collectionownership/EnhancedForDisposalLoopResolver.java
  • checker/src/main/java/org/checkerframework/checker/collectionownership/IndexedForDisposalLoopMatcher.java
  • checker/src/main/java/org/checkerframework/checker/collectionownership/WhileDisposalLoopMatcher.java

@iamsanjaymalakar
Copy link
Copy Markdown
Member Author

iamsanjaymalakar commented Apr 28, 2026

@msridhar here's a summary of the new structure and the design changes:

  • Introduced a new hook in GenericAnnotatedTypeFactory#postCFGConstruction that runs within #analyze after the CFG has been constructed but before dataflow analysis CFAbstractAnalysis#perfomAnalysis runs. This was necessay for disposal loop's metadata as they require CFG facts, but need to run before the flow analysis.

Before

Disposal loop handling was split across multiple phases and checkers:

  • MustCallVisitor did the initial discovery of disposal loops , but some loops (enhanced for, while) required CFG to extract loop facts like the back-edge info so it stored the matched loops into RLCCalledMethodsAnnotatedTypeFactory
  • RLCC owned the per-method loop state (some of them still incomplete), seeded the initial CM store for temp variables to TOP inRLCCalledMethodsTransfer#initialStore so that the iterated collection element (col.get(i), pop(), next(), etc) are in CM store before RLCC CM analysis ran.
  • The missing information from the stored disposal loops are filled-in during RLCC post analysis when CFG is available
  • During RLCC post analysis MCCA was invoked to analyze the disposal loops and store the called-methods on the iterated collection element over the loop body on every path, stored the resulting called-methods into CO-owned lookup maps
  • Later, during CollectionOwnershipTransfer and MCCA used those CO maps (dispoal loop -> called-methods) to refine the collection to @OwningCollectionWithoutObligation.

After current change

  • The old disposal logic from MustCallVisitor moved into a custom scanner (DisposalLoopScanner) which CollectionOwnershipAnnotatedTypeFactory owns. The CollectionOwnershipAnnotatedTypeFactory also owns the data-structures for storing the discovered disposal loops.
  • The disposal loop scanner is triggered from RLCCalledMethodsAnnotatedTypeFactory#postCFGConstruction and the disposal loops are stored in CollectionOwnershipAnnotatedTypeFactory. The scanner is triggered from RLCCalledMethodsAnnotatedTypeFactory#postCFGConstruction for two reasons:
    • The loop discovery and resolution of all loop facts (mainly backedge) needs both AST and CFG, so the new hook is the right place
    • For the discovered loops, the initial CM store needs to be seeded for the loop iterated collection elements that are temp variables ((col.get(i), pop(), next(), etc)). So the discovery must happend before RLCC's stores are initialized.
  • RLCCalledMethodsTransfer.#initialStore reads the discovered loops stored in CO and initialized the store like before.
  • CollectionOwnershipAnnotatedTypeFactory#postCFGConstruction invoked the MCCA on the disposal loops to analyze and store the called-methods on the iterated collection element. So after this CO has for each 'disposal loop -> called-methods on the iterated-elelement on loop body' information.
  • During CO flow, CollectionOwnershipTransfer reads those CO-owned disposal loop facts (loop -> called-methods) and compares the loop’s called-method set against the required @MustCall set of the iterated element. If the loop covers the required must-call methods, the collection is refined to @OwningCollectionWithoutObligation.

So now CO owns eveything regarding disposal loops. Only the trigger for loop discovery is from RLCC.

Copy link
Copy Markdown
Contributor

@msridhar msridhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's going to take me a while to do a full review. Here are some very initial comments.

@Override
public AnnotationVisitor visitAnnotation(String descriptor, boolean visible) {
@SuppressWarnings("nullness:override.return") // ASM lacks (some?) @Nullable annotations
public @Nullable AnnotationVisitor visitAnnotation(String descriptor, boolean visible) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do the changes in this file belong in this PR?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. Removed suppressions from this file.

* iterates over a resource collection and may call the disposal method, e.g., close() on the
* iterated resource.
*/
public class DisposalLoop {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this class can just be a record


/**
* Stores the resolved CFG and AST facts for a disposal loop. A disposal loop is a loop that
* iterates over a resource collection and may call the disposal method, e.g., close() on the
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why "may call"? I would think that we would reserve the name DisposalLoop for a loop that we've shown in fact is a disposal loop?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If in fact this is not definitely a disposal loop, maybe we should name this class DisposalLoopInfo to indicate it has all the info for a disposal loop, but it's not necessarily a "verified" disposal loop?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

* @return the disposal loops discovered in {@code tree}
*/
public Set<DisposalLoop> scanTree(Tree tree) {
disposalLoops.clear();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it important for performance to be able to use a DisposalLoopScanner object multiple times? Or does it make the code cleaner?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reuse is not important here. One DisposalLoopScanner is created per method CFG and use it only once. I’ve simplified the scanner and removed the clearing.

}

for (IPair<Block, @Nullable TypeMirror> successorAndExceptionType :
CollectionOwnershipUtils.getSuccessorsExceptIgnoredExceptions(currentBlock, coAtf)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to pay attention to ignored exceptions here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for the ignored exception filtering is that one enhanced-for AST can correspond to more than one CFG occurrence around finally.

For example:

try {
  ...
} finally {
  for (Resource r : resources) {
    r.close();
  }
}

there is only one enhanced-for in the AST, but the CFG can contain multiple copies of that loop for two control-flow paths (normal+exceptional) through the finally.

The current resolver does a CFG search and returns the first matching occurrence it finds. If that search also follows exceptional successors, it can reach the exceptional copy of the loop before the normal copy, and then bind to the wrong CFG occurrence of the same source loop.

I've added comments for this in the code.

Comment on lines +117 to +121
if (loop == null) {
throw new BugInCF(
"MethodInvocationNode.iterableExpression should be non-null iff"
+ " MethodInvocationNode.enhancedForLoop is non-null");
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need this check here? Given we check if loop != tree immediately after?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to keep the nullness checker happy, it shouldn't trigger in code.

*/
private @Nullable DisposalLoop resolveEnhancedForLoop(
MethodInvocationNode methodInvocationNode, @FindDistinct EnhancedForLoopTree tree) {
if (methodInvocationNode.getIterableExpression() == null) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a pretty big function. Does it make sense to split it into smaller functions? Or, can we add some comments in the different parts to explain the different phases of the check?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added comments for different phases.

Copy link
Copy Markdown
Contributor

@msridhar msridhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More comments. I think it's best to address these comments before I continue further.

import org.checkerframework.dataflow.cfg.node.Node;
import org.checkerframework.javacutil.TreeUtils;

/** Matches indexed `for` {@link DisposalLoop}'s that iterates over a resource collection. */
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/** Matches indexed `for` {@link DisposalLoop}'s that iterates over a resource collection. */
/** Matches indexed `for` {@link DisposalLoop}s that iterate over a resource collection. */

if (loopBodyStatements == null) {
return null;
}
StatementTree init = tree.getInitializer().get(0);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to check if getInitializer() returns a list of length 1?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thanks for noticing.

}
StatementTree init = tree.getInitializer().get(0);
ExpressionTree condition = TreeUtils.withoutParens(tree.getCondition());
ExpressionStatementTree update = tree.getUpdate().get(0);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to check if getUpdate() returns a list of length 1?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is not addressed

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check is added on top of this method.

* @param expr an expression
* @return the name of the referenced identifier, or {@code null} if none
*/
static Name getNameFromExpressionTree(ExpressionTree expr) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
static Name getNameFromExpressionTree(ExpressionTree expr) {
static @Nullable Name getNameFromExpressionTree(ExpressionTree expr) {

Comment on lines +178 to +179
AtomicBoolean blockIsIllegal = new AtomicBoolean(false);
final ExpressionTree[] collectionElementTree = {null};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than creating these mutable locals to expose to the anonymous class, you can make scanner a named class with fields. Then, you can run the scan and then read out the result value via getters at the end.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed.

Comment on lines +228 to +230
for (StatementTree stmt : statements) {
scanner.scan(stmt, null);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function can just take the ForLoopTree as a parameter, and then apply its scanner to tree.getStatement(), rather than looping over the statements

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed.

* @return a list of statements for {@code statement}, or {@code null} if {@code statement} is
* {@code null}
*/
static @Nullable List<? extends StatementTree> asStatementList(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on looking at the callers of this method, I don't think it's needed. In both cases the calling code seems cleaner without it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

Comment on lines +246 to +248
if (tree == null || index == null) {
return false;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not allow null parameters and just get rid of these checks

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
annotation-file-utilities/src/main/java/org/checkerframework/afu/scenelib/util/coll/LinkedHashKeyedSet.java (2)

110-140: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

add(V) and addAll() lack @NotOwningCollection on their receivers, but both delegate to the @NotOwningCollection-constrained 3-arg add.

add(V) (line 124) calls add(o, THROW_EXCEPTION, IGNORE) on this, and addAll() (line 136) calls add(V). Since the 3-arg add declares @NotOwningCollection LinkedHashKeyedSet<K,V> this, passing an un-annotated (or @OwningCollection) receiver to it will produce a checker type error. Both callers need matching receiver annotations:

🛡️ Proposed fix
   `@Override`
-  public boolean add(V o) {
+  public boolean add(`@NotOwningCollection` LinkedHashKeyedSet<K, V> this, V o) {
     return add(o, THROW_EXCEPTION, IGNORE) == null;
   }

   `@Override`
-  public boolean addAll(Collection<? extends V> c) {
+  public boolean addAll(`@NotOwningCollection` LinkedHashKeyedSet<K, V> this, Collection<? extends V> c) {
     boolean changed = false;
     for (V o : c) {
       changed |= add(o);
     }
     return changed;
   }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@annotation-file-utilities/src/main/java/org/checkerframework/afu/scenelib/util/coll/LinkedHashKeyedSet.java`
around lines 110 - 140, The no-argument add(V) and addAll(Collection<? extends
V>) methods call the 3-arg add which requires a `@NotOwningCollection` receiver,
so annotate their receivers to match: add(V) and addAll(...) should declare
"@NotOwningCollection LinkedHashKeyedSet<K,V> this" (same annotation style as
the 3-arg add) so the checker sees a consistent non-owning receiver when
delegating to add(o, THROW_EXCEPTION, IGNORE) and add(o) respectively.

152-155: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

replace(V) bypasses the @NotOwningCollection receiver guard by calling theMap.put() directly.

add(V, int, int) enforces @NotOwningCollection on this before inserting into theMap, but replace(V) performs the same theMap.put() mutation without any receiver annotation — making it an unguarded insertion path on potentially @OwningCollection instances.

🛡️ Proposed fix
   `@Override`
-  public V replace(V v) {
+  public V replace(`@NotOwningCollection` LinkedHashKeyedSet<K, V> this, V v) {
     return theMap.put(keyer.getKeyFor(v), v);
   }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@annotation-file-utilities/src/main/java/org/checkerframework/afu/scenelib/util/coll/LinkedHashKeyedSet.java`
around lines 152 - 155, The replace(V) method mutates theMap directly and
bypasses the `@NotOwningCollection` receiver guard enforced by add(V,int,int); fix
by making replace use the same guarded insertion path: either annotate the
receiver of replace with `@NotOwningCollection` and/or call the existing
add(V,int,int) helper (e.g., delegate to add(v, /*pos*/ -1, /*flags*/ -1) or the
appropriate parameters) so the same guard runs before inserting, or if there is
an internal check method (e.g., checkNotOwningCollection or similar) invoke that
before calling theMap.put(keyer.getKeyFor(v), v) to ensure the same protection
as add.
♻️ Duplicate comments (6)
checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java (4)

1033-1035: ⚠️ Potential issue | 🟠 Major

Match the parameter slot by name content, not Name identity.

Name instances are not guaranteed to be reference-equal, so this can miss the matching parameter and fail to propagate the declaration annotation to the use site. Compare with contentEquals(...) or .equals(...) instead.

Suggested fix
-            if (params.get(i).getSimpleName() == elt.getSimpleName()) {
+            if (params.get(i).getSimpleName().contentEquals(elt.getSimpleName())) {
               type.replaceAnnotation(paramTypes.get(i).getAnnotationInHierarchy(TOP));
               break;
             }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java`
around lines 1033 - 1035, The code compares parameter names using reference
equality (params.get(i).getSimpleName() == elt.getSimpleName()), which can fail;
change the comparison to a content-based string comparison (e.g.,
params.get(i).getSimpleName().contentEquals(elt.getSimpleName()) or
.equals(...)) so the loop in CollectionOwnershipAnnotatedTypeFactory correctly
finds the matching parameter and then calls
type.replaceAnnotation(paramTypes.get(i).getAnnotationInHierarchy(TOP)) to
propagate the declaration annotation.

701-703: ⚠️ Potential issue | 🔴 Critical

Mirror the BugInCF guard used by isResourceCollection(Tree).

mcAtf.getAnnotatedType(tree) can still throw for unsupported tree shapes, and this helper currently crashes analysis instead of yielding the intended “no obligations” result. It should catch BugInCF and return null here as well.

Suggested fix
   public List<String> getMustCallValuesOfResourceCollectionComponent(Tree tree) {
-    return getMustCallValuesOfResourceCollectionComponent(mcAtf.getAnnotatedType(tree));
+    if (tree == null) {
+      return null;
+    }
+    try {
+      return getMustCallValuesOfResourceCollectionComponent(mcAtf.getAnnotatedType(tree));
+    } catch (BugInCF e) {
+      return null;
+    }
   }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java`
around lines 701 - 703, The helper
getMustCallValuesOfResourceCollectionComponent(Tree) can throw BugInCF when
calling mcAtf.getAnnotatedType(tree) for unsupported tree shapes; update this
method to mirror the guard used by isResourceCollection(Tree) by catching
BugInCF around the mcAtf.getAnnotatedType(tree) call and returning null (i.e.,
no obligations) when caught so analysis does not crash; reference the
mcAtf.getAnnotatedType(tree) invocation and the BugInCF exception in your
change.

619-630: ⚠️ Potential issue | 🟠 Major

Use the stub-aware ownership helpers for inserted elements.

Element.getAnnotation(...) bypasses stub files, so a stubbed @NotOwning or @Owning parameter is misclassified here and collection-obligation transfer becomes unsound. These checks should go through rlAtf.hasNotOwning(...) and rlAtf.hasOwning(...) instead.

Suggested fix
-    if (insertedElement != null && insertedElement.getAnnotation(NotOwning.class) != null) {
+    if (insertedElement != null && rlAtf.hasNotOwning(insertedElement)) {
       return CollectionMutatorArgumentKind.DEFINITELY_NON_OWNING;
     }
@@
     if (insertedElement != null && insertedElement.getKind() == ElementKind.PARAMETER) {
-      return insertedElement.getAnnotation(Owning.class) == null
+      return !rlAtf.hasOwning(insertedElement)
           ? CollectionMutatorArgumentKind.DEFINITELY_NON_OWNING
           : CollectionMutatorArgumentKind.MAY_BE_OWNING;
     }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java`
around lines 619 - 630, The code in CollectionOwnershipAnnotatedTypeFactory uses
TreeUtils.elementFromTree(insertedArgumentTree) and then calls
Element.getAnnotation(...) which ignores stub annotations; replace those direct
calls with the stub-aware helpers rlAtf.hasNotOwning(insertedElement) and
rlAtf.hasOwning(insertedElement) when determining the
CollectionMutatorArgumentKind for an insertedElement: use
rlAtf.hasNotOwning(...) to return DEFINITELY_NON_OWNING, and for parameters use
rlAtf.hasOwning(...) to choose between MAY_BE_OWNING and DEFINITELY_NON_OWNING
instead of Element.getAnnotation(...).

514-516: ⚠️ Potential issue | 🟡 Minor

Guard elt before dereferencing it.

This method still calls elt.asType() before any null check, so a null input will NPE here even though the sibling collection-field helpers now return false for null.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java`
around lines 514 - 516, The method isOwningCollectionParameter dereferences elt
via elt.asType() before guarding against null; add a null-check at the start of
isOwningCollectionParameter and return false if elt is null (or otherwise ensure
elt != null) so you don't call elt.asType() on a null reference; move the
isResourceCollection(elt.asType()) call to after the null check (and after
confirming elt.getKind() == ElementKind.PARAMETER if you prefer) to prevent the
NPE and keep behavior consistent with the collection-field helper methods.
checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakUtils.java (2)

452-462: ⚠️ Potential issue | 🟠 Major

Only treat explicit @MustCallUnknown as manual here.

getPrimaryAnnotationInHierarchy(mcAtf.TOP) returns the effective qualifier, so an unannotated type that merely defaults to top also makes this helper return true. That preserves @MustCallUnknown in places that were never explicitly annotated. Check the underlying TypeMirror annotations, or delegate to the TypeMirror overload instead.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakUtils.java`
around lines 452 - 462, hasManualMustCallUnknownAnno currently uses
annotatedTypeMirror.getPrimaryAnnotationInHierarchy(mcAtf.TOP) which returns the
effective qualifier (including defaults) and thus treats implicit TOP as an
explicit `@MustCallUnknown`; instead ensure only explicit annotations are
considered by examining the underlying TypeMirror annotations or by delegating
to the TypeMirror-based helper: use the AnnotatedTypeMirror's underlying
TypeMirror (or call the existing hasManualMustCallUnknownAnno(TypeMirror,
MustCallAnnotatedTypeFactory) if present) and check its annotation mirrors for
MustCallUnknown via AnnotationUtils.areSameByName, rather than using
getPrimaryAnnotationInHierarchy(mcAtf.TOP).

225-229: ⚠️ Potential issue | 🔴 Critical

Don't cast MustCallNoCreatesMustCallForChecker to CollectionOwnershipChecker.

This branch still accepts MustCallNoCreatesMustCallForChecker, but then unconditionally casts referenceChecker to CollectionOwnershipChecker. Any lookup that reaches this helper from the no-CMCF checker will fail with ClassCastException; route that case through the parent/subchecker chain instead.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakUtils.java`
around lines 225 - 229, The current branch treats a
MustCallNoCreatesMustCallForChecker as if it were a CollectionOwnershipChecker
and unconditionally casts referenceChecker, causing ClassCastException; modify
the logic in ResourceLeakUtils where referenceChecker is checked (the code that
compares className and returns a CollectionOwnershipAnnotatedTypeFactory) so
that when className equals "MustCallNoCreatesMustCallForChecker" you do not cast
it directly to CollectionOwnershipChecker but instead walk the checker
parent/subchecker chain from referenceChecker to locate the actual
CollectionOwnershipChecker instance (e.g., loop via getParent()/subchecker
accessors until you find an instance of CollectionOwnershipChecker), then cast
that found instance to CollectionOwnershipChecker and return its getTypeFactory
as a CollectionOwnershipAnnotatedTypeFactory, and handle the case where no such
parent exists safely (null or error).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/IndexedForDisposalLoopMatcher.java`:
- Around line 131-169: In nameOfCollectionThatAllElementsAreCalledOn, only
accept a condition whose operator is the strict less-than operator; currently
any BinaryTree with a RHS of collection.size() is accepted (allowing <=, !=,
etc.). Add a guard that condition.getKind() == Tree.Kind.LESS_THAN before
checking operands and size access so only patterns like i < collection.size()
match; keep the existing checks tying the left operand and increment to the init
variable and the RHS size check (including TreeUtils.isSizeAccess,
coAtf.isResourceCollection, and
CollectionOwnershipUtils.getNameFromExpressionTree).
- Around line 149-155: In IndexedForDisposalLoopMatcher, replace all reference
comparisons of javax.lang.model.element.Name objects with value comparisons:
change checks like initVarName != ((IdentifierTree)
condition.getLeftOperand()).getName(), initVarName != ((IdentifierTree)
inc.getExpression()).getName(), and the other Name comparisons in this class to
use !name1.equals(name2) (and name1.equals(name2) for equality) instead of
!=/==; ensure you update every occurrence (including the comparisons referenced
around initVarName, condition.getLeftOperand(), inc.getExpression(), and the
other Name variables mentioned in the review) so logical matching of loop and
collection names uses .equals() consistently.

---

Outside diff comments:
In
`@annotation-file-utilities/src/main/java/org/checkerframework/afu/scenelib/util/coll/LinkedHashKeyedSet.java`:
- Around line 110-140: The no-argument add(V) and addAll(Collection<? extends
V>) methods call the 3-arg add which requires a `@NotOwningCollection` receiver,
so annotate their receivers to match: add(V) and addAll(...) should declare
"@NotOwningCollection LinkedHashKeyedSet<K,V> this" (same annotation style as
the 3-arg add) so the checker sees a consistent non-owning receiver when
delegating to add(o, THROW_EXCEPTION, IGNORE) and add(o) respectively.
- Around line 152-155: The replace(V) method mutates theMap directly and
bypasses the `@NotOwningCollection` receiver guard enforced by add(V,int,int); fix
by making replace use the same guarded insertion path: either annotate the
receiver of replace with `@NotOwningCollection` and/or call the existing
add(V,int,int) helper (e.g., delegate to add(v, /*pos*/ -1, /*flags*/ -1) or the
appropriate parameters) so the same guard runs before inserting, or if there is
an internal check method (e.g., checkNotOwningCollection or similar) invoke that
before calling theMap.put(keyer.getKeyFor(v), v) to ensure the same protection
as add.

---

Duplicate comments:
In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java`:
- Around line 1033-1035: The code compares parameter names using reference
equality (params.get(i).getSimpleName() == elt.getSimpleName()), which can fail;
change the comparison to a content-based string comparison (e.g.,
params.get(i).getSimpleName().contentEquals(elt.getSimpleName()) or
.equals(...)) so the loop in CollectionOwnershipAnnotatedTypeFactory correctly
finds the matching parameter and then calls
type.replaceAnnotation(paramTypes.get(i).getAnnotationInHierarchy(TOP)) to
propagate the declaration annotation.
- Around line 701-703: The helper
getMustCallValuesOfResourceCollectionComponent(Tree) can throw BugInCF when
calling mcAtf.getAnnotatedType(tree) for unsupported tree shapes; update this
method to mirror the guard used by isResourceCollection(Tree) by catching
BugInCF around the mcAtf.getAnnotatedType(tree) call and returning null (i.e.,
no obligations) when caught so analysis does not crash; reference the
mcAtf.getAnnotatedType(tree) invocation and the BugInCF exception in your
change.
- Around line 619-630: The code in CollectionOwnershipAnnotatedTypeFactory uses
TreeUtils.elementFromTree(insertedArgumentTree) and then calls
Element.getAnnotation(...) which ignores stub annotations; replace those direct
calls with the stub-aware helpers rlAtf.hasNotOwning(insertedElement) and
rlAtf.hasOwning(insertedElement) when determining the
CollectionMutatorArgumentKind for an insertedElement: use
rlAtf.hasNotOwning(...) to return DEFINITELY_NON_OWNING, and for parameters use
rlAtf.hasOwning(...) to choose between MAY_BE_OWNING and DEFINITELY_NON_OWNING
instead of Element.getAnnotation(...).
- Around line 514-516: The method isOwningCollectionParameter dereferences elt
via elt.asType() before guarding against null; add a null-check at the start of
isOwningCollectionParameter and return false if elt is null (or otherwise ensure
elt != null) so you don't call elt.asType() on a null reference; move the
isResourceCollection(elt.asType()) call to after the null check (and after
confirming elt.getKind() == ElementKind.PARAMETER if you prefer) to prevent the
NPE and keep behavior consistent with the collection-field helper methods.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakUtils.java`:
- Around line 452-462: hasManualMustCallUnknownAnno currently uses
annotatedTypeMirror.getPrimaryAnnotationInHierarchy(mcAtf.TOP) which returns the
effective qualifier (including defaults) and thus treats implicit TOP as an
explicit `@MustCallUnknown`; instead ensure only explicit annotations are
considered by examining the underlying TypeMirror annotations or by delegating
to the TypeMirror-based helper: use the AnnotatedTypeMirror's underlying
TypeMirror (or call the existing hasManualMustCallUnknownAnno(TypeMirror,
MustCallAnnotatedTypeFactory) if present) and check its annotation mirrors for
MustCallUnknown via AnnotationUtils.areSameByName, rather than using
getPrimaryAnnotationInHierarchy(mcAtf.TOP).
- Around line 225-229: The current branch treats a
MustCallNoCreatesMustCallForChecker as if it were a CollectionOwnershipChecker
and unconditionally casts referenceChecker, causing ClassCastException; modify
the logic in ResourceLeakUtils where referenceChecker is checked (the code that
compares className and returns a CollectionOwnershipAnnotatedTypeFactory) so
that when className equals "MustCallNoCreatesMustCallForChecker" you do not cast
it directly to CollectionOwnershipChecker but instead walk the checker
parent/subchecker chain from referenceChecker to locate the actual
CollectionOwnershipChecker instance (e.g., loop via getParent()/subchecker
accessors until you find an instance of CollectionOwnershipChecker), then cast
that found instance to CollectionOwnershipChecker and return its getTypeFactory
as a CollectionOwnershipAnnotatedTypeFactory, and handle the case where no such
parent exists safely (null or error).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9a587578-7873-47bc-90b1-261e9b1b33f5

📥 Commits

Reviewing files that changed from the base of the PR and between 2ab0f26 and 4f134eb.

📒 Files selected for processing (16)
  • annotation-file-utilities/src/main/java/org/checkerframework/afu/scenelib/util/coll/LinkedHashKeyedSet.java
  • checker-qual/src/main/java/org/checkerframework/checker/collectionownership/qual/CreatesCollectionObligation.java
  • checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnalysis.java
  • checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java
  • checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipTransfer.java
  • checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipUtils.java
  • checker/src/main/java/org/checkerframework/checker/collectionownership/DisposalLoopInfo.java
  • checker/src/main/java/org/checkerframework/checker/collectionownership/DisposalLoopScanner.java
  • checker/src/main/java/org/checkerframework/checker/collectionownership/EnhancedForDisposalLoopResolver.java
  • checker/src/main/java/org/checkerframework/checker/collectionownership/IndexedForDisposalLoopMatcher.java
  • checker/src/main/java/org/checkerframework/checker/collectionownership/WhileDisposalLoopMatcher.java
  • checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
  • checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakUtils.java
  • checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java
  • checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsTransfer.java
  • framework-test/src/main/java/org/checkerframework/framework/test/diagnostics/JavaDiagnosticReader.java

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (2)
checker/src/main/java/org/checkerframework/checker/collectionownership/EnhancedForDisposalLoopResolver.java (2)

187-198: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reject non-matching CFG shapes instead of throwing BugInCF.

Lines 187-198 abort analysis on CFG-shape mismatches. In this resolver, a shape mismatch should be treated as “not a match” and return null, to avoid crashing on benign variants.

Proposed fix
     Block blockContainingLoopCondition = loopConditionNode.getBlock();
     if (blockContainingLoopCondition.getSuccessors().size() != 1) {
-      throw new BugInCF(
-          "loop condition has: "
-              + blockContainingLoopCondition.getSuccessors().size()
-              + " successors instead of 1.");
+      return null;
     }
     Block maybeConditionalBlock = blockContainingLoopCondition.getSuccessors().iterator().next();
     if (!(maybeConditionalBlock instanceof ConditionalBlock conditionalBlock)) {
-      throw new BugInCF(
-          "loop condition successor is not ConditionalBlock, but: "
-              + maybeConditionalBlock.getClass());
+      return null;
     }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/EnhancedForDisposalLoopResolver.java`
around lines 187 - 198, The code in EnhancedForDisposalLoopResolver currently
throws BugInCF when the CFG shape differs (checks on
blockContainingLoopCondition.getSuccessors() and instance-of ConditionalBlock on
maybeConditionalBlock); change these to treat shape mismatches as non-matches by
returning null instead of throwing: after verifying
blockContainingLoopCondition.getSuccessors().size() != 1 or that
maybeConditionalBlock is not a ConditionalBlock, simply return null (do not
throw), so the resolver signals “no match” safely.

154-156: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Use value equality for Name, not reference equality.

Line 155 compares Name instances with ==, which can miss matches and cause false negatives when identifying the loop-variable assignment.

Proposed fix
-        isAssignmentOfLoopVariable =
-            iteratorVariableDeclaration.getName() == loopVariable.getName();
+        isAssignmentOfLoopVariable =
+            iteratorVariableDeclaration.getName().equals(loopVariable.getName());
In Java (javax.lang.model.element.Name / com.sun.source tree names), should semantic name comparison use equals(...) instead of == ?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/EnhancedForDisposalLoopResolver.java`
around lines 154 - 156, The comparison in EnhancedForDisposalLoopResolver uses
reference equality for Name objects: change the check that sets
isAssignmentOfLoopVariable (which currently does
iteratorVariableDeclaration.getName() == loopVariable.getName()) to use value
equality (e.g., call equals or Objects.equals) so Name instances are compared
semantically; update the isAssignmentOfLoopVariable assignment to use
iteratorVariableDeclaration.getName().equals(loopVariable.getName()) or
Objects.equals(...) to avoid NPEs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Duplicate comments:
In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/EnhancedForDisposalLoopResolver.java`:
- Around line 187-198: The code in EnhancedForDisposalLoopResolver currently
throws BugInCF when the CFG shape differs (checks on
blockContainingLoopCondition.getSuccessors() and instance-of ConditionalBlock on
maybeConditionalBlock); change these to treat shape mismatches as non-matches by
returning null instead of throwing: after verifying
blockContainingLoopCondition.getSuccessors().size() != 1 or that
maybeConditionalBlock is not a ConditionalBlock, simply return null (do not
throw), so the resolver signals “no match” safely.
- Around line 154-156: The comparison in EnhancedForDisposalLoopResolver uses
reference equality for Name objects: change the check that sets
isAssignmentOfLoopVariable (which currently does
iteratorVariableDeclaration.getName() == loopVariable.getName()) to use value
equality (e.g., call equals or Objects.equals) so Name instances are compared
semantically; update the isAssignmentOfLoopVariable assignment to use
iteratorVariableDeclaration.getName().equals(loopVariable.getName()) or
Objects.equals(...) to avoid NPEs.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: ed428866-e711-4513-acfb-9308f3a206b0

📥 Commits

Reviewing files that changed from the base of the PR and between 4f134eb and ef2595d.

📒 Files selected for processing (1)
  • checker/src/main/java/org/checkerframework/checker/collectionownership/EnhancedForDisposalLoopResolver.java

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (2)
checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakUtils.java (2)

227-230: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

MustCallNoCreatesMustCallForChecker will hit a ClassCastException here.

The branch accepts MustCallNoCreatesMustCallForChecker but unconditionally casts referenceChecker to CollectionOwnershipChecker. Since MustCallNoCreatesMustCallForChecker lives in org.checkerframework.checker.mustcall and does not extend CollectionOwnershipChecker, any call reaching this helper from the no-CMCF checker will throw at runtime. Route it through getParentChecker() like the MustCallChecker case below.

🔧 Suggested fix
-    if ("CollectionOwnershipChecker".equals(className)
-        || "MustCallNoCreatesMustCallForChecker".equals(className)) {
+    if ("CollectionOwnershipChecker".equals(className)) {
       return (CollectionOwnershipAnnotatedTypeFactory)
           ((CollectionOwnershipChecker) referenceChecker).getTypeFactory();
-    } else if ("RLCCalledMethodsChecker".equals(className)) {
-      return getCollectionOwnershipAnnotatedTypeFactory(referenceChecker.getParentChecker());
-    } else if ("MustCallChecker".equals(className)) {
+    } else if ("RLCCalledMethodsChecker".equals(className)
+        || "MustCallChecker".equals(className)
+        || "MustCallNoCreatesMustCallForChecker".equals(className)) {
       return getCollectionOwnershipAnnotatedTypeFactory(referenceChecker.getParentChecker());
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakUtils.java`
around lines 227 - 230, The current branch treats
"MustCallNoCreatesMustCallForChecker" the same as "CollectionOwnershipChecker"
and casts referenceChecker to CollectionOwnershipChecker, causing a
ClassCastException; update the logic in ResourceLeakUtils so that when className
equals "MustCallNoCreatesMustCallForChecker" you cast referenceChecker to
MustCallNoCreatesMustCallForChecker, call getParentChecker() on it, then cast
that parent to CollectionOwnershipChecker and obtain its type factory (as a
CollectionOwnershipAnnotatedTypeFactory) — mirror the approach used for
MustCallChecker in the code below rather than casting referenceChecker directly.

454-467: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

hasManualMustCallUnknownAnno still treats a defaulted TOP as if it were manually written.

getPrimaryAnnotationInHierarchy(mcAtf.TOP) returns the effective qualifier, so a type that merely defaults to @MustCallUnknown (e.g., an unannotated T upper bound) also makes this return true. Inspect the underlying TypeMirror's annotation mirrors instead so only explicitly written @MustCallUnknown matches — this also lets you reuse the TypeMirror overload above.

🔧 Suggested fix
   public static boolean hasManualMustCallUnknownAnno(
       AnnotatedTypeMirror annotatedTypeMirror, MustCallAnnotatedTypeFactory mcAtf) {
     if (annotatedTypeMirror == null) {
       return false;
     }
-    AnnotationMirror manualMcAnno = annotatedTypeMirror.getPrimaryAnnotationInHierarchy(mcAtf.TOP);
-    if (manualMcAnno == null) {
-      return false;
-    }
-    if (AnnotationUtils.areSameByName(manualMcAnno, MustCallUnknown.class.getCanonicalName())) {
-      return true;
-    }
-    return false;
+    return hasManualMustCallUnknownAnno(annotatedTypeMirror.getUnderlyingType());
   }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakUtils.java`
around lines 454 - 467, The current
hasManualMustCallUnknownAnno(AnnotatedTypeMirror, MustCallAnnotatedTypeFactory)
incorrectly treats defaulted TOP qualifiers as manual because it uses
getPrimaryAnnotationInHierarchy(mcAtf.TOP); instead, obtain the underlying
TypeMirror from the AnnotatedTypeMirror (e.g.,
annotatedTypeMirror.getUnderlyingType()) and inspect its annotation mirrors for
an explicit `@MustCallUnknown`, or simply delegate to the existing TypeMirror
overload hasManualMustCallUnknownAnno(TypeMirror, MustCallAnnotatedTypeFactory)
so only annotations actually present on the TypeMirror (via
getAnnotationMirrors()) are considered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakUtils.java`:
- Around line 264-303: Update the Javadoc for both overloads of isCollection
(the methods public static boolean isCollection(Element element,
AnnotatedTypeFactory atf) and public static boolean isCollection(TypeMirror
type)) to accurately reflect that they treat java.lang.Iterable,
java.util.Iterator, and java.util.Map subtypes as "collections"; ensure the
param/return descriptions and summary mention Map as well as Iterable and
Iterator. Also add a short NOTE or TODO in the Javadoc suggesting reconsidering
the method name since isCollection no longer maps to java.util.Collection
semantics so callers are aware of the widened definition.

---

Duplicate comments:
In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakUtils.java`:
- Around line 227-230: The current branch treats
"MustCallNoCreatesMustCallForChecker" the same as "CollectionOwnershipChecker"
and casts referenceChecker to CollectionOwnershipChecker, causing a
ClassCastException; update the logic in ResourceLeakUtils so that when className
equals "MustCallNoCreatesMustCallForChecker" you cast referenceChecker to
MustCallNoCreatesMustCallForChecker, call getParentChecker() on it, then cast
that parent to CollectionOwnershipChecker and obtain its type factory (as a
CollectionOwnershipAnnotatedTypeFactory) — mirror the approach used for
MustCallChecker in the code below rather than casting referenceChecker directly.
- Around line 454-467: The current
hasManualMustCallUnknownAnno(AnnotatedTypeMirror, MustCallAnnotatedTypeFactory)
incorrectly treats defaulted TOP qualifiers as manual because it uses
getPrimaryAnnotationInHierarchy(mcAtf.TOP); instead, obtain the underlying
TypeMirror from the AnnotatedTypeMirror (e.g.,
annotatedTypeMirror.getUnderlyingType()) and inspect its annotation mirrors for
an explicit `@MustCallUnknown`, or simply delegate to the existing TypeMirror
overload hasManualMustCallUnknownAnno(TypeMirror, MustCallAnnotatedTypeFactory)
so only annotations actually present on the TypeMirror (via
getAnnotationMirrors()) are considered.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: ae3b1c35-dd8f-4451-8ded-4b2a44832340

📥 Commits

Reviewing files that changed from the base of the PR and between ef2595d and c2ecad3.

📒 Files selected for processing (1)
  • checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakUtils.java

Comment on lines +264 to +303
/**
* Returns true if the given Element is a java.lang.Iterable or java.util.Iterator type by
* checking whether the raw type of the element is assignable from either. Returns false if
* element is null, or has no valid type.
*
* @param element the element
* @param atf an AnnotatedTypeFactory to get the annotated type of the element
* @return true if the given element is a Java.lang.Iterable or Java.util.Iterator type
*/
public static boolean isCollection(Element element, AnnotatedTypeFactory atf) {
if (element == null) {
return false;
}
AnnotatedTypeMirror elementTypeMirror = atf.getAnnotatedType(element).getErased();
if (elementTypeMirror == null || elementTypeMirror.getUnderlyingType() == null) {
return false;
}
return isCollection(elementTypeMirror.getUnderlyingType());
}

/**
* Returns true if the given {@link TypeMirror} is a java.lang.Iterable or java.util.Iterator
* subclass. This is determined by getting the class of the TypeMirror and checking whether it is
* assignable from either.
*
* @param type the TypeMirror
* @return true if type is a java.lang.Iterable or java.util.Iterator
*/
public static boolean isCollection(TypeMirror type) {
if (type == null) {
return false;
}
Class<?> elementRawType = TypesUtils.getClassFromType(type);
if (elementRawType == null) {
return false;
}
return Iterable.class.isAssignableFrom(elementRawType)
|| Iterator.class.isAssignableFrom(elementRawType)
|| Map.class.isAssignableFrom(elementRawType);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Javadoc for isCollection omits Map.

Both overloads now also classify Map subtypes as collections (line 302), but the Javadoc on lines 264-272 and 284-291 still says only "java.lang.Iterable or java.util.Iterator". This is misleading for callers who rely on the doc to reason about resource-collection scope (especially given the recent decision to treat maps as resource collections). Update the Javadoc — and the method name should probably be reconsidered too, since isCollection no longer matches java.util.Collection semantics either.

🔧 Suggested doc fix
-   * Returns true if the given Element is a java.lang.Iterable or java.util.Iterator type by
-   * checking whether the raw type of the element is assignable from either. Returns false if
-   * element is null, or has no valid type.
+   * Returns true if the given Element's raw type is assignable from {`@link` java.lang.Iterable},
+   * {`@link` java.util.Iterator}, or {`@link` java.util.Map} (i.e., a "resource collection" in the
+   * sense used by this checker). Returns false if element is null, or has no valid type.
...
-   * Returns true if the given {`@link` TypeMirror} is a java.lang.Iterable or java.util.Iterator
-   * subclass. This is determined by getting the class of the TypeMirror and checking whether it is
-   * assignable from either.
+   * Returns true if the given {`@link` TypeMirror} is assignable from {`@link` java.lang.Iterable},
+   * {`@link` java.util.Iterator}, or {`@link` java.util.Map}.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakUtils.java`
around lines 264 - 303, Update the Javadoc for both overloads of isCollection
(the methods public static boolean isCollection(Element element,
AnnotatedTypeFactory atf) and public static boolean isCollection(TypeMirror
type)) to accurately reflect that they treat java.lang.Iterable,
java.util.Iterator, and java.util.Map subtypes as "collections"; ensure the
param/return descriptions and summary mention Map as well as Iterable and
Iterator. Also add a short NOTE or TODO in the Javadoc suggesting reconsidering
the method name since isCollection no longer maps to java.util.Collection
semantics so callers are aware of the widened definition.

@iamsanjaymalakar iamsanjaymalakar requested a review from msridhar May 8, 2026 04:39
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (2)
checker/src/main/java/org/checkerframework/checker/collectionownership/WhileDisposalLoopMatcher.java (2)

443-446: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Use value equality for Name comparisons, not reference equality.

javax.lang.model.element.Name is compared with == at Line 445, Line 525, Line 529, and Line 606. This can cause false negatives when matching loop variables.

Proposed fix
@@
-    return receiverName != null && receiverName == headerVarName;
+    return receiverName != null && receiverName.equals(headerVarName);
@@
-      if (assignedVariable == headerVarName) {
+      if (assignedVariable.equals(headerVarName)) {
         illegal = true;
       }
       if (collectionVarNameForInvalidation != null
-          && assignedVariable == collectionVarNameForInvalidation) {
+          && assignedVariable.equals(collectionVarNameForInvalidation)) {
         illegal = true;
       }
@@
-          if (mutatedVariable == headerVarName) {
+          if (headerVarName.equals(mutatedVariable)) {
             illegal = true;
             return null;
           }

Also applies to: 520-532, 600-607

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/WhileDisposalLoopMatcher.java`
around lines 443 - 446, Replace reference equality checks on
javax.lang.model.element.Name with value equality: where the code currently does
"receiverName == headerVarName" (and the similar comparisons at other
occurrences), change to use receiverName.equals(headerVarName) (or
receiverName.contentEquals(headerVarName)) after a null check. Update the
comparisons in WhileDisposalLoopMatcher (the variables receiverName and
headerVarName and the other Name comparisons referenced) to use .equals(...) so
value equality is used instead of '=='.

711-716: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Select loop update block from back-edge source, not target.

At Line 715, bestLoopUpdateBlock is assigned backEdge.targetBlock. For a back edge (source -> target), the update/tail location is the source block; using target usually points to the loop header.

Proposed fix
@@
       if (naturalLoop.size() < bestLoopSize) {
         bestLoopSize = naturalLoop.size();
-        bestLoopUpdateBlock = backEdge.targetBlock;
+        bestLoopUpdateBlock = backEdge.sourceBlock;
       }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/WhileDisposalLoopMatcher.java`
around lines 711 - 716, The code in WhileDisposalLoopMatcher that chooses the
loop update block uses backEdge.targetBlock (which is the loop header) but
should use the back-edge source (the loop tail/update); change the assignment
where bestLoopUpdateBlock is set (in the block that compares naturalLoop.size()
to bestLoopSize) to assign backEdge.sourceBlock instead of backEdge.targetBlock
so bestLoopUpdateBlock points to the loop update location.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Duplicate comments:
In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/WhileDisposalLoopMatcher.java`:
- Around line 443-446: Replace reference equality checks on
javax.lang.model.element.Name with value equality: where the code currently does
"receiverName == headerVarName" (and the similar comparisons at other
occurrences), change to use receiverName.equals(headerVarName) (or
receiverName.contentEquals(headerVarName)) after a null check. Update the
comparisons in WhileDisposalLoopMatcher (the variables receiverName and
headerVarName and the other Name comparisons referenced) to use .equals(...) so
value equality is used instead of '=='.
- Around line 711-716: The code in WhileDisposalLoopMatcher that chooses the
loop update block uses backEdge.targetBlock (which is the loop header) but
should use the back-edge source (the loop tail/update); change the assignment
where bestLoopUpdateBlock is set (in the block that compares naturalLoop.size()
to bestLoopSize) to assign backEdge.sourceBlock instead of backEdge.targetBlock
so bestLoopUpdateBlock points to the loop update location.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 2daf41a8-bbc1-491c-81b8-7213ce5c436b

📥 Commits

Reviewing files that changed from the base of the PR and between ac65c40 and edd7f0b.

📒 Files selected for processing (1)
  • checker/src/main/java/org/checkerframework/checker/collectionownership/WhileDisposalLoopMatcher.java

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
framework/src/main/java/org/checkerframework/framework/flow/CFAbstractTransfer.java (1)

1523-1531: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

insertIntoStoresPermitNonDeterministic is inconsistent with the updated insertIntoStores.

After this PR, the two sibling helpers diverge in three ways:

insertIntoStores insertIntoStoresPermitNonDeterministic
Modifier protected (instance) protected static
Parameter type TransferResult<V, S> TransferResult<CFValue, CFStore>
Overridable

A subclass with V ≠ CFValue or S ≠ CFStore still cannot pass its native TransferResult<V, S> to insertIntoStoresPermitNonDeterministic without an unchecked cast, which is precisely the problem that was just fixed in insertIntoStores. The companion method should be updated to match:

♻️ Proposed fix
-  protected static void insertIntoStoresPermitNonDeterministic(
-      TransferResult<CFValue, CFStore> result, JavaExpression target, AnnotationMirror newAnno) {
+  protected void insertIntoStoresPermitNonDeterministic(
+      TransferResult<V, S> result, JavaExpression target, AnnotationMirror newAnno) {
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@framework/src/main/java/org/checkerframework/framework/flow/CFAbstractTransfer.java`
around lines 1523 - 1531, Change insertIntoStoresPermitNonDeterministic to match
insertIntoStores: make it an instance (non-static) protected method, generic in
the same type parameters (e.g., <V,S extends CFStore>) and accept
TransferResult<V,S> so subclasses can pass their native TransferResult without
casts; keep the same body that calls
result.getThenStore()/getElseStore()/getRegularStore() and invokes
insertValuePermitNondeterministic on those stores so the method remains
overridable.
♻️ Duplicate comments (9)
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java (8)

539-606: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Keep CollectionObligation immutable and hash-consistent.

This subtype adds mustCallMethod to equals(...) but not to hashCode(), and the field is still mutable. Because obligations are stored in hash-based sets, mutating the field or hashing without it can break visited-state deduplication and obligation lookups.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`
around lines 539 - 606, CollectionObligation introduces a mutable mustCallMethod
field and includes it in equals(...) but not hashCode(), breaking hash-based
collections; make mustCallMethod private final, update all constructors
(CollectionObligation(...), CollectionObligation(Obligation,...), and
fromTree(...)/getReplacement(...) usages) to set the final field, and add an
overridden hashCode() that combines super.hashCode() with
mustCallMethod.hashCode() (or Objects.hash(super.hashCode(), mustCallMethod)) so
equality and hashing are consistent and the class is immutable.

907-919: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Treat missing component must-call values as “no collection obligation”.

getMustCallValuesOfResourceCollectionComponent(...) can legitimately return null here. Throwing BugInCF turns a valid @OwningCollection return into an analyzer crash instead of “no element obligations”.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`
around lines 907 - 919, The code currently throws BugInCF when
getMustCallValuesOfResourceCollectionComponent(...) returns null; change this so
a null return is treated as “no collection obligation” instead of an analyzer
bug: in MustCallConsistencyAnalyzer, locate the block that assigns
mustCallValues from
getMustCallValuesOfResourceCollectionComponent(node.getTree()) and remove the
throw of BugInCF; if mustCallValues is null simply skip adding
CollectionObligation entries to obligations (i.e., do nothing), while preserving
the existing logic that adds CollectionObligation for each mustCallMethod when
mustCallValues is non-null and ResourceLeakUtils.isIterator(node.getType()) is
false (keep references to tmpVarAsResourceAlias and MethodExitKind.ALL
unchanged).

3713-3716: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Null-check cmValOfIce before calling getCalledMethods(...).

store.getValue(ice) can be null. Calling getCalledMethods(cmValOfIce) unconditionally turns that case into a NullPointerException instead of “no called-methods fact for the iterated element”.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`
around lines 3713 - 3716, NullPointer risk: store.getValue(ice) can be null;
check before calling getCalledMethods. Fix by testing AccumulationValue
cmValOfIce = store.getValue(ice) for null and only call
getCalledMethods(cmValOfIce) when non-null; if cmValOfIce is null, leave
calledMethodsAfterThisBlock as empty/null (the current “no called-methods”
semantics) so you avoid a NullPointerException. Ensure references to
store.getValue, cmValOfIce, getCalledMethods(...), and
calledMethodsAfterThisBlock are updated accordingly.

3548-3559: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

A single unverified back edge must invalidate the loop.

When analyzeTypeOfCollectionElement(...) returns null, this branch just skips the intersection and keeps facts learned from other back edges. That can still certify a disposal loop even though one path failed to prove the required calls.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`
around lines 3548 - 3559, The code currently ignores a null result from
analyzeTypeOfCollectionElement and keeps prior facts, which is unsound; when
analyzeTypeOfCollectionElement(...) returns null for any back edge, you must
invalidate the whole loop by clearing/setting calledMethodsInLoop to null (or
another "unknown" sentinel) so the loop cannot be certified; update the branch
in MustCallConsistencyAnalyzer where calledMethodsAfterBlock is checked to set
calledMethodsInLoop = null (rather than skipping) whenever
calledMethodsAfterBlock == null, ensuring all back edges must succeed to
preserve facts.

2100-2105: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Treat OwningCollectionBottom as the nullable-field case, not a checker bug.

In this helper, bottom is exactly the “field currently holds null” state. Throwing BugInCF here turns a safe null -> owning collection assignment into an analyzer crash.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`
around lines 2100 - 2105, The code currently throws a BugInCF when lhsCoType ==
CollectionOwnershipType.OwningCollectionBottom; instead treat
OwningCollectionBottom as the nullable-field case (not an internal error).
Replace the throw of BugInCF in MustCallConsistencyAnalyzer with the same
handling used for the nullable-field branch (i.e., handle OwningCollectionBottom
like OwningCollectionNullableField: allow a null->owning-collection assignment
and perform the same downstream updates/returns instead of crashing). Ensure you
reference lhsCoType and CollectionOwnershipType.OwningCollectionBottom and
remove the BugInCF throw.

2048-2053: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guard component must-call lookups before calling .get(0).

getMustCallValuesOfResourceCollectionComponent(...) can return null or an empty list. Both error paths can currently throw before reporting the intended diagnostic.

Also applies to: 2131-2135

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`
around lines 2048 - 2053, Retrieve the result of
coAtf.getMustCallValuesOfResourceCollectionComponent(lhs.getTree()) into a local
variable, check it for null and emptiness before calling .get(0), and pass a
safe fallback (e.g., null or lhs.getTree()) into checker.reportError when no
element exists; update the call site in MustCallConsistencyAnalyzer where
checker.reportError(...) currently uses .get(0) directly (and the similar
occurrence noted later) so the code does not NPE on null/empty lists.

965-973: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don’t consume the inserted element when checkEnclosingMethodIsCreatesMustCallFor(...) fails.

If that check reports missing/incompatible.creates.mustcall.for, this path still removes the caller-side obligation. That makes the analysis behave as though ownership moved into the field anyway and can mask a later leak on the original value.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`
around lines 965 - 973, The code unconditionally calls
consumeInsertedArgumentObligationIfSingleElementInsert(...) even when
checkEnclosingMethodIsCreatesMustCallFor(...) reported a
missing/incompatible.creates.mustcall.for error, which incorrectly drops the
caller-side obligation; change the logic so that when receiverIsOwningField is
true you first determine whether the enclosing-method check succeeded and only
then transfer/consume the obligation. Concretely, make
checkEnclosingMethodIsCreatesMustCallFor(...) return a boolean (or add a local
success flag set by it) when called for receiverNode/enclosingMethodTree, and
call consumeInsertedArgumentObligationIfSingleElementInsert(obligations, node)
only if receiverIsOwningField is false or the enclosing-method check returned
success; ensure references to receiverIsOwningField,
cmAtf.getPath(node.getTree()), TreePathUtil.enclosingMethod(currentPath),
checkEnclosingMethodIsCreatesMustCallFor,
consumeInsertedArgumentObligationIfSingleElementInsert, obligations, and node
are used to locate and implement this change.

933-958: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Model @CreatesCollectionObligation only on the normal edge, and enforce it on all later exits.

This helper runs for every successor edge, but it ignores whether the call actually returned normally. On exceptional edges it can fabricate a collection obligation, and on normal edges CollectionObligation.fromTree(...) limits enforcement to NORMAL_RETURN, so a later exceptional exit can skip a real obligation.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`
around lines 933 - 958, The code currently models `@CreatesCollectionObligation`
on every successor edge and uses CollectionObligation.fromTree which limits
enforcement to NORMAL_RETURN, allowing exceptional exits to skip real
obligations; update addObligationsForCreatesCollectionObligationAnno to only
create obligations when the invocation node represents a normal-return edge
(i.e., the call actually returned normally via the successor/edge on the node)
by checking the node/edge kind (use the MethodInvocationNode or its edge info
like the successor/edge) and, when creating the obligation, ensure the
obligation is built to be enforced on all later exits (not limited to
NORMAL_RETURN) — adjust the use of CollectionObligation.fromTree or construct
the obligation with an enforcement kind that covers all exits so exceptional
paths cannot bypass it; keep references to
coAtf.isCreatesCollectionObligationMethod, receiverNode removal helpers,
coAtf.getMustCallValuesOfResourceCollectionComponent, and
CollectionObligation.fromTree when making the change.
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallInference.java (1)

311-313: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Use one WholeProgramInference instance throughout this inference pass.

This method now reads WPI from coAtf, but helpers reached from the same flow still call resourceLeakAtf.getWholeProgramInference(). If those ATFs ever return different instances, inferred annotations for the same method/class can be written inconsistently.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallInference.java`
around lines 311 - 313, The code creates a WholeProgramInference via
CollectionOwnershipAnnotatedTypeFactory (coAtf.getWholeProgramInference()) but
other helpers still call resourceLeakAtf.getWholeProgramInference(), which can
produce different instances; unify on one WPI instance by retrieving it once
(WholeProgramInference wpi = coAtf.getWholeProgramInference()) in
MustCallInference and pass that wpi into any helper methods (or store it on this
instance) instead of letting helpers call
resourceLeakAtf.getWholeProgramInference(); update all places that currently
call resourceLeakAtf.getWholeProgramInference() to accept/use the passed/stored
wpi (referencing MustCallInference, resourceLeakAtf,
CollectionOwnershipAnnotatedTypeFactory coAtf, and WholeProgramInference wpi).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`:
- Around line 1083-1086: The code is adding owning-collection return obligations
even for calls that may throw; mirror the exceptional-edge suppression used in
updateObligationsWithInvocationResult(...) by skipping creation of these
obligations when the current CFG successor is an exceptional edge. Concretely,
guard the call to addObligationsForOwningCollectionReturn(obligations, node)
(and the subsequent
addObligationsForCreatesCollectionObligationAnno(obligations,
methodInvocationNode) for MethodInvocationNode) with a check that the current
successor is not an exceptional successor (i.e., only add obligations when the
invocation result is on a normal successor), using the same CFG/edge-test used
by updateObligationsWithInvocationResult to detect exceptional edges.

In
`@dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java`:
- Line 825: Replace the plain static long uid with an AtomicLong (e.g.,
protected static final AtomicLong uid = new AtomicLong(0)) and use
uid.getAndIncrement() wherever uid was read to produce synthetic names (match
the established pattern used by MustCallTransfer.uniqueName()); if cross-CFG
uniqueness is not intentionally required, ensure per-CFG determinism by
resetting or scoping the AtomicLong per translation (or instead use
TreeUtils.treeUids for identity if the change truly needs cross-tree/global
uniqueness) and document the choice in the commit message.

---

Outside diff comments:
In
`@framework/src/main/java/org/checkerframework/framework/flow/CFAbstractTransfer.java`:
- Around line 1523-1531: Change insertIntoStoresPermitNonDeterministic to match
insertIntoStores: make it an instance (non-static) protected method, generic in
the same type parameters (e.g., <V,S extends CFStore>) and accept
TransferResult<V,S> so subclasses can pass their native TransferResult without
casts; keep the same body that calls
result.getThenStore()/getElseStore()/getRegularStore() and invokes
insertValuePermitNondeterministic on those stores so the method remains
overridable.

---

Duplicate comments:
In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`:
- Around line 539-606: CollectionObligation introduces a mutable mustCallMethod
field and includes it in equals(...) but not hashCode(), breaking hash-based
collections; make mustCallMethod private final, update all constructors
(CollectionObligation(...), CollectionObligation(Obligation,...), and
fromTree(...)/getReplacement(...) usages) to set the final field, and add an
overridden hashCode() that combines super.hashCode() with
mustCallMethod.hashCode() (or Objects.hash(super.hashCode(), mustCallMethod)) so
equality and hashing are consistent and the class is immutable.
- Around line 907-919: The code currently throws BugInCF when
getMustCallValuesOfResourceCollectionComponent(...) returns null; change this so
a null return is treated as “no collection obligation” instead of an analyzer
bug: in MustCallConsistencyAnalyzer, locate the block that assigns
mustCallValues from
getMustCallValuesOfResourceCollectionComponent(node.getTree()) and remove the
throw of BugInCF; if mustCallValues is null simply skip adding
CollectionObligation entries to obligations (i.e., do nothing), while preserving
the existing logic that adds CollectionObligation for each mustCallMethod when
mustCallValues is non-null and ResourceLeakUtils.isIterator(node.getType()) is
false (keep references to tmpVarAsResourceAlias and MethodExitKind.ALL
unchanged).
- Around line 3713-3716: NullPointer risk: store.getValue(ice) can be null;
check before calling getCalledMethods. Fix by testing AccumulationValue
cmValOfIce = store.getValue(ice) for null and only call
getCalledMethods(cmValOfIce) when non-null; if cmValOfIce is null, leave
calledMethodsAfterThisBlock as empty/null (the current “no called-methods”
semantics) so you avoid a NullPointerException. Ensure references to
store.getValue, cmValOfIce, getCalledMethods(...), and
calledMethodsAfterThisBlock are updated accordingly.
- Around line 3548-3559: The code currently ignores a null result from
analyzeTypeOfCollectionElement and keeps prior facts, which is unsound; when
analyzeTypeOfCollectionElement(...) returns null for any back edge, you must
invalidate the whole loop by clearing/setting calledMethodsInLoop to null (or
another "unknown" sentinel) so the loop cannot be certified; update the branch
in MustCallConsistencyAnalyzer where calledMethodsAfterBlock is checked to set
calledMethodsInLoop = null (rather than skipping) whenever
calledMethodsAfterBlock == null, ensuring all back edges must succeed to
preserve facts.
- Around line 2100-2105: The code currently throws a BugInCF when lhsCoType ==
CollectionOwnershipType.OwningCollectionBottom; instead treat
OwningCollectionBottom as the nullable-field case (not an internal error).
Replace the throw of BugInCF in MustCallConsistencyAnalyzer with the same
handling used for the nullable-field branch (i.e., handle OwningCollectionBottom
like OwningCollectionNullableField: allow a null->owning-collection assignment
and perform the same downstream updates/returns instead of crashing). Ensure you
reference lhsCoType and CollectionOwnershipType.OwningCollectionBottom and
remove the BugInCF throw.
- Around line 2048-2053: Retrieve the result of
coAtf.getMustCallValuesOfResourceCollectionComponent(lhs.getTree()) into a local
variable, check it for null and emptiness before calling .get(0), and pass a
safe fallback (e.g., null or lhs.getTree()) into checker.reportError when no
element exists; update the call site in MustCallConsistencyAnalyzer where
checker.reportError(...) currently uses .get(0) directly (and the similar
occurrence noted later) so the code does not NPE on null/empty lists.
- Around line 965-973: The code unconditionally calls
consumeInsertedArgumentObligationIfSingleElementInsert(...) even when
checkEnclosingMethodIsCreatesMustCallFor(...) reported a
missing/incompatible.creates.mustcall.for error, which incorrectly drops the
caller-side obligation; change the logic so that when receiverIsOwningField is
true you first determine whether the enclosing-method check succeeded and only
then transfer/consume the obligation. Concretely, make
checkEnclosingMethodIsCreatesMustCallFor(...) return a boolean (or add a local
success flag set by it) when called for receiverNode/enclosingMethodTree, and
call consumeInsertedArgumentObligationIfSingleElementInsert(obligations, node)
only if receiverIsOwningField is false or the enclosing-method check returned
success; ensure references to receiverIsOwningField,
cmAtf.getPath(node.getTree()), TreePathUtil.enclosingMethod(currentPath),
checkEnclosingMethodIsCreatesMustCallFor,
consumeInsertedArgumentObligationIfSingleElementInsert, obligations, and node
are used to locate and implement this change.
- Around line 933-958: The code currently models `@CreatesCollectionObligation` on
every successor edge and uses CollectionObligation.fromTree which limits
enforcement to NORMAL_RETURN, allowing exceptional exits to skip real
obligations; update addObligationsForCreatesCollectionObligationAnno to only
create obligations when the invocation node represents a normal-return edge
(i.e., the call actually returned normally via the successor/edge on the node)
by checking the node/edge kind (use the MethodInvocationNode or its edge info
like the successor/edge) and, when creating the obligation, ensure the
obligation is built to be enforced on all later exits (not limited to
NORMAL_RETURN) — adjust the use of CollectionObligation.fromTree or construct
the obligation with an enforcement kind that covers all exits so exceptional
paths cannot bypass it; keep references to
coAtf.isCreatesCollectionObligationMethod, receiverNode removal helpers,
coAtf.getMustCallValuesOfResourceCollectionComponent, and
CollectionObligation.fromTree when making the change.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallInference.java`:
- Around line 311-313: The code creates a WholeProgramInference via
CollectionOwnershipAnnotatedTypeFactory (coAtf.getWholeProgramInference()) but
other helpers still call resourceLeakAtf.getWholeProgramInference(), which can
produce different instances; unify on one WPI instance by retrieving it once
(WholeProgramInference wpi = coAtf.getWholeProgramInference()) in
MustCallInference and pass that wpi into any helper methods (or store it on this
instance) instead of letting helpers call
resourceLeakAtf.getWholeProgramInference(); update all places that currently
call resourceLeakAtf.getWholeProgramInference() to accept/use the passed/stored
wpi (referencing MustCallInference, resourceLeakAtf,
CollectionOwnershipAnnotatedTypeFactory coAtf, and WholeProgramInference wpi).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: e8fb2a3c-78f4-4fc3-abbd-1d4769fab0ef

📥 Commits

Reviewing files that changed from the base of the PR and between edd7f0b and e4a57d7.

📒 Files selected for processing (8)
  • annotation-file-utilities/src/main/java/org/checkerframework/afu/scenelib/util/coll/WrapperMap.java
  • checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
  • checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallInference.java
  • dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java
  • framework/src/main/java/org/checkerframework/framework/flow/CFAbstractTransfer.java
  • framework/src/main/java/org/checkerframework/framework/stub/StubGenerator.java
  • framework/src/main/java/org/checkerframework/framework/type/QualifierHierarchy.java
  • javacutil/src/main/java/org/checkerframework/javacutil/AnnotationMirrorMap.java

Comment on lines +1083 to +1086
addObligationsForOwningCollectionReturn(obligations, node);

if (node instanceof MethodInvocationNode methodInvocationNode) {
addObligationsForCreatesCollectionObligationAnno(obligations, methodInvocationNode);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Skip owning-collection return obligations on exceptional successors.

updateObligationsWithInvocationResult(...) has special exceptional-edge suppression for temp vars, but addObligationsForOwningCollectionReturn(...) does not. As written, a throwing call can still create CollectionObligations for a return value that never exists.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`
around lines 1083 - 1086, The code is adding owning-collection return
obligations even for calls that may throw; mirror the exceptional-edge
suppression used in updateObligationsWithInvocationResult(...) by skipping
creation of these obligations when the current CFG successor is an exceptional
edge. Concretely, guard the call to
addObligationsForOwningCollectionReturn(obligations, node) (and the subsequent
addObligationsForCreatesCollectionObligationAnno(obligations,
methodInvocationNode) for MethodInvocationNode) with a check that the current
successor is not an exceptional successor (i.e., only add obligations when the
invocation result is on a normal successor), using the same CFG/edge-test used
by updateObligationsWithInvocationResult to detect exceptional edges.


/** The UID for the next unique name. */
protected long uid = 0;
protected static long uid = 0;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find references to synthetic names produced by uniqueName() in expected/golden test files and docs.
rg -nP '#num\d+' -g '!**/build/**' -g '!**/.git/**' -C2

# Find any other uses of `uniqueName(` to understand the call surface.
rg -nP '\buniqueName\s*\(' -C2

# Look for parallelism in annotation processing / CFG building that could expose the race.
rg -nP '(parallelStream|ForkJoin|ExecutorService|newFixedThreadPool|Thread\s*\()' -g '*.java' -C1

Repository: typetools/checker-framework

Length of output: 22000


🌐 Web query:

Does javac's annotation processing API permit running multiple Processor instances or CFG translations concurrently within a single compilation task?

💡 Result:

No, javac's annotation processing API does not permit running multiple Processor instances or CFG translations concurrently within a single compilation task. The API is designed for sequential, cooperative processing across multiple rounds, with processors invoked one after another in a single thread. Official documentation explicitly states: "The tool environment is not required to support annotation processors that access environmental resources, either per round or cross-round, in a multi-threaded fashion."[1][2][3][4] Processors are single instances per class, initialized once via init and called sequentially in process based on annotation claiming order.[5][3][6] Javac source snippets show no parallel execution for annotation processing; parallelism efforts like JDK-8046129 target compilation phases, not processors.[7][8]

Citations:


🏁 Script executed:

# Check the type and implementation of uid in MustCallTransfer
grep -n "uid" dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java

Repository: typetools/checker-framework

Length of output: 148


🏁 Script executed:

# Check MustCallTransfer's uid field declaration
grep -B5 -A5 "protected String uniqueName" checker/src/main/java/org/checkerframework/checker/mustcall/MustCallTransfer.java

Repository: typetools/checker-framework

Length of output: 462


🏁 Script executed:

# Look for imports of AtomicLong in MustCallTransfer to confirm type
grep -n "import.*Atomic\|uid.*=" checker/src/main/java/org/checkerframework/checker/mustcall/MustCallTransfer.java | head -20

Repository: typetools/checker-framework

Length of output: 176


🏁 Script executed:

# Check if Expected.txt files are generated or manually maintained
find . -name "Expected.txt" -type f | head -5 | xargs head -5

Repository: typetools/checker-framework

Length of output: 1133


🏁 Script executed:

# Look for test generation patterns or regeneration comments
grep -r "regenerate\|generate.*golden\|Expected" dataflow/tests/ --include="*.java" | head -10

Repository: typetools/checker-framework

Length of output: 53


uid should be AtomicLong, not plain static long, to preserve determinism and follow established patterns in this codebase.

This change introduces two problems:

  1. Non-determinism and test breakage: Synthetic names like switch#num0, iter#numN, etc. previously restarted at 0 per CFG method, making them deterministic within a single method. The golden test file dataflow/tests/java22/unnamed-pattern/Expected.txt hardcodes switch#num0 throughout, confirming tests depend on this numbering. Now the counter is global and never resets between compilation tasks, breaking reproducible builds and any golden-file / CFG-visualization tests that include these names.

  2. Thread safety / cross-task state leak: While javac's annotation processing API is single-threaded per task, the JVM may run multiple independent compilation tasks in parallel (e.g., parallel Maven/Gradle builds, IDE incremental checks). With a plain static long, the counter persists and increments across all tasks in the same JVM, causing synthetic names to diverge between builds. Use AtomicLong with getAndIncrement() — this pattern is already established in the codebase (see MustCallTransfer.uniqueName()).

  3. PR scope: It's unclear why an RLC/Collection Ownership PR needs globally unique synthetic variable names. If downstream RLCC seeding or the disposal-loop scanner genuinely requires cross-CFG identity, please justify it in the commit message and consider keying off TreeUtils.treeUids instead.

🛡️ Suggested fix
-  protected static long uid = 0;
+  private static final java.util.concurrent.atomic.AtomicLong uid =
+      new java.util.concurrent.atomic.AtomicLong();

   protected String uniqueName(String prefix) {
-    return prefix + "#num" + uid++;
+    return prefix + "#num" + uid.getAndIncrement();
   }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java`
at line 825, Replace the plain static long uid with an AtomicLong (e.g.,
protected static final AtomicLong uid = new AtomicLong(0)) and use
uid.getAndIncrement() wherever uid was read to produce synthetic names (match
the established pattern used by MustCallTransfer.uniqueName()); if cross-CFG
uniqueness is not intentionally required, ensure per-CFG determinism by
resetting or scoping the AtomicLong per translation (or instead use
TreeUtils.treeUids for identity if the change truly needs cross-tree/global
uniqueness) and document the choice in the commit message.

Comment on lines +120 to +122
if (expr == null) {
return null;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not addressed?

Comment on lines +230 to +234
if (loop == null) {
throw new BugInCF(
"MethodInvocationNode.iterableExpression should be non-null iff"
+ " MethodInvocationNode.enhancedForLoop is non-null");
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't think this check should be needed here. I don't see why it impacts the nullness checker.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, the loop == tree takes care of everything. thanks for pointing it out.

}
StatementTree init = tree.getInitializer().get(0);
ExpressionTree condition = TreeUtils.withoutParens(tree.getCondition());
ExpressionStatementTree update = tree.getUpdate().get(0);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is not addressed

@Override
public @Nullable Void scan(@Nullable Tree tree, Void p) {
// Short-circuit the scanner if the collection/index variable is mutated.
if (bodyIsIllegal || tree == null) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we allow for tree == null?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, this was unnecessay.


@Override
public @Nullable Void scan(@Nullable Iterable<? extends Tree> trees, Void p) {
if (bodyIsIllegal || trees == null) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we allow for trees == null?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, this was unnecessay.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

♻️ Duplicate comments (12)
checker/src/main/java/org/checkerframework/checker/collectionownership/EnhancedForDisposalLoopResolver.java (2)

154-156: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Name comparison still uses == (duplicate, not actually fixed).

This was previously flagged and marked "Addressed in commit 4f134eb", but line 155 still compares javax.lang.model.element.Name instances with ==. Name implementations are not guaranteed to be interned across compilation contexts, so this can produce false negatives and silently reject otherwise-matching loops. Use contentEquals (or Objects.equals) instead.

🛡️ Proposed fix
       if ((candidateNode instanceof AssignmentNode)
           && (candidateNode.getTree() instanceof VariableTree iteratorVariableDeclaration)) {
         loopVariableNode = ((AssignmentNode) candidateNode).getTarget();
         isAssignmentOfLoopVariable =
-            iteratorVariableDeclaration.getName() == loopVariable.getName();
+            iteratorVariableDeclaration.getName().contentEquals(loopVariable.getName());
       }
javax.lang.model.element.Name interning guarantees and contentEquals usage
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/EnhancedForDisposalLoopResolver.java`
around lines 154 - 156, The comparison of javax.lang.model.element.Name is using
== which can fail; in EnhancedForDisposalLoopResolver replace the identity
comparison between iteratorVariableDeclaration.getName() and
loopVariable.getName() (the assignment that sets isAssignmentOfLoopVariable)
with a content-based comparison such as
iteratorVariableDeclaration.getName().contentEquals(loopVariable.getName()) or
Objects.equals(...) so names are compared by content rather than reference.

186-198: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

CFG shape mismatch still throws BugInCF (duplicate, not actually fixed).

This was previously flagged and marked "Addressed in commit 4f134eb", but both shape checks still throw BugInCF instead of returning null. A non-matching CFG should reject the candidate, not abort analysis — particularly because match is invoked from disposal-loop discovery and any unexpected desugaring will turn into a checker crash on user code. Return null and let the caller skip this loop.

🛡️ Proposed fix
     Block blockContainingLoopCondition = loopConditionNode.getBlock();
     if (blockContainingLoopCondition.getSuccessors().size() != 1) {
-      throw new BugInCF(
-          "loop condition has: "
-              + blockContainingLoopCondition.getSuccessors().size()
-              + " successors instead of 1.");
+      return null;
     }
     Block maybeConditionalBlock = blockContainingLoopCondition.getSuccessors().iterator().next();
     if (!(maybeConditionalBlock instanceof ConditionalBlock conditionalBlock)) {
-      throw new BugInCF(
-          "loop condition successor is not ConditionalBlock, but: "
-              + maybeConditionalBlock.getClass());
+      return null;
     }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/EnhancedForDisposalLoopResolver.java`
around lines 186 - 198, The match method in EnhancedForDisposalLoopResolver
currently throws BugInCF for CFG shape mismatches; change both failure branches
to return null instead so a non-matching CFG rejects the candidate rather than
aborting analysis: replace the first throw in the
blockContainingLoopCondition.getSuccessors().size() != 1 branch with a null
return, and replace the second throw in the !(maybeConditionalBlock instanceof
ConditionalBlock) branch with a null return (these references appear around
loopConditionNode, blockContainingLoopCondition, maybeConditionalBlock, and
conditionalBlock).
checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipTransfer.java (2)

324-324: 🧹 Nitpick | 🔵 Trivial | 💤 Low value

Prefer isEmpty() over size() == 0.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipTransfer.java`
at line 324, The local variable isDiamond is computed by comparing
getTypeArguments().size() == 0; replace that check with
getTypeArguments().isEmpty() to use the preferred idiom; update the assignment
to isDiamond in CollectionOwnershipTransfer (the expression using
node.getTree().getTypeArguments()) accordingly.

256-294: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

argElem can be null; argElem.getKind() will NPE on common call shapes.

The previous fix added the argType == null short-circuit at Line 260, but argElem (Line 264) is still nullable — TreeUtils.elementFromTree returns null for literals, parenthesized expressions, temp-var trees, and other non-element-bearing trees. When transferOwnership == true for such an argument, Line 287 will NPE before the field-vs-non-field branching even runs.

Proposed fix
       if (transferOwnership) {
-        if (argElem.getKind().isField()) {
+        if (argElem != null && argElem.getKind().isField()) {
           checker.reportError(
               arg.getTree(), "transfer.owningcollection.field.ownership", arg.getTree().toString());
         } else {
           replaceInStores(res, argJE, atypeFactory.NOTOWNINGCOLLECTION);
         }
       }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipTransfer.java`
around lines 256 - 294, When transferOwnership is true, guard the use of argElem
returned by TreeUtils.elementFromTree: check whether argElem is null before
calling argElem.getKind(); if argElem is non-null and
argElem.getKind().isField() call checker.reportError(arg.getTree(),
"transfer.owningcollection.field.ownership", arg.getTree().toString()),
otherwise treat it as the non-field case and call replaceInStores(res, argJE,
atypeFactory.NOTOWNINGCOLLECTION); this prevents an NPE when
TreeUtils.elementFromTree returns null for literals/parenthesized/temp trees.
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java (8)

1706-1745: 🧹 Nitpick | 🔵 Trivial | 💤 Low value

Redundant guard and dead commented-out code.

  • Line 1683 already gates this call on isLoopBodyAnalysis; the inner if (!isLoopBodyAnalysis) return; (Lines 1706-1708) is redundant.
  • The // I don't think we need a tempVar here ... block (Lines 1724-1731) was previously flagged. If tempVar handling is genuinely unnecessary, please delete the dead lines; if uncertain, replace them with a structured TODO instead of leaving an inline justification followed by commented code.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`
around lines 1706 - 1745, Remove the redundant early-return guard and the dead
commented-out tempVar code inside the loopBody analysis block: delete the inner
"if (!isLoopBodyAnalysis) return;" check (the outer call already ensures
isLoopBodyAnalysis) and either remove the commented tempVar branch or replace it
with a single-line TODO noting potential temp-var handling; update the
treeForAlias assignment (currently node.getTree()) to remain as the active code
path and keep references to symbols Obligation, ResourceAlias, rhsExpr, lhsVar,
node.getTree(), typeFactory.isTempVar, and typeFactory.getTreeForTempVar in the
comment/TODO if you leave a placeholder so future reviewers can reintroduce
tempVar handling cleanly.

909-914: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Still throws on a null component must-call list.

getMustCallValuesOfResourceCollectionComponent(...) is documented to legitimately return null for owning collections over non-resource element types. Throwing BugInCF here turns a valid program into an analyzer crash; the safe behavior is to skip creating CollectionObligations and return.

Proposed fix
         List<String> mustCallValues =
             coAtf.getMustCallValuesOfResourceCollectionComponent(node.getTree());
-        if (mustCallValues == null) {
-          throw new BugInCF(
-              "List of MustCall values of component type is null for OwningCollection return value:"
-                  + " "
-                  + node);
-        }
-        if (!ResourceLeakUtils.isIterator(node.getType())) {
+        if (mustCallValues == null || ResourceLeakUtils.isIterator(node.getType())) {
+          return;
+        }
           for (String mustCallMethod : mustCallValues) {
             obligations.add(
                 new CollectionObligation(
                     mustCallMethod, ImmutableSet.of(tmpVarAsResourceAlias), MethodExitKind.ALL));
           }
-        }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`
around lines 909 - 914, The current code in MustCallConsistencyAnalyzer (inside
the logic handling OwningCollection return values) throws a BugInCF when
mustCallValues is null; instead handle the documented legitimate null return
from getMustCallValuesOfResourceCollectionComponent(...) by skipping creation of
CollectionObligation objects and returning early. Specifically, in the block
that checks mustCallValues, remove the BugInCF throw and replace it with a
no-op/early return so that when mustCallValues == null the method does not
create CollectionObligation instances and continues safely.

2098-2106: ⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy lift

OwningCollectionBottom LHS should not crash analysis.

When lhsCoType == OwningCollectionBottom, the field currently holds null, which the surrounding logic explicitly treats as a legal target for assignment. Throwing BugInCF here aborts analysis of valid programs; the bottom case should fall through to the “first assignment / safe overwrite” path instead.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`
around lines 2098 - 2106, The bug: MustCallConsistencyAnalyzer currently throws
BugInCF when lhsCoType == CollectionOwnershipType.OwningCollectionBottom even
though OwningCollectionBottom represents null and should be treated as a legal
RHS/LHS that falls through to the "first assignment / safe overwrite" handling;
fix by removing the throw for the OwningCollectionBottom branch in the method
where the check occurs (the block that currently checks lhsCoType ==
CollectionOwnershipType.OwningCollectionBottom) and let execution continue to
the existing first-assignment/safe-overwrite logic, keeping or updating the
comment to explain that OwningCollectionBottom is allowed and should not abort
analysis.

1083-1087: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Owning-collection return obligations are still added on exceptional edges.

updateObligationsForInvocation runs once per outgoing edge (including each exceptional successor), but neither addObligationsForOwningCollectionReturn nor addObligationsForCreatesCollectionObligationAnno consults exceptionType. A throwing call therefore fabricates CollectionObligations for a return value that does not exist on that path, mirroring the temp-var suppression already implemented in updateObligationsWithInvocationResult.

Proposed fix
-    addObligationsForOwningCollectionReturn(obligations, node);
-
-    if (node instanceof MethodInvocationNode methodInvocationNode) {
-      addObligationsForCreatesCollectionObligationAnno(obligations, methodInvocationNode);
+    if (exceptionType == null) {
+      addObligationsForOwningCollectionReturn(obligations, node);
+      if (node instanceof MethodInvocationNode methodInvocationNode) {
+        addObligationsForCreatesCollectionObligationAnno(obligations, methodInvocationNode);
+      }
     }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`
around lines 1083 - 1087, The current code always calls
addObligationsForOwningCollectionReturn and
addObligationsForCreatesCollectionObligationAnno from
updateObligationsForInvocation for every outgoing edge, which fabricates
CollectionObligation entries on exceptional edges; update
updateObligationsForInvocation to consult the exceptionType parameter (or
otherwise detect an exceptional successor) and skip calling
addObligationsForOwningCollectionReturn and
addObligationsForCreatesCollectionObligationAnno when exceptionType is non-null
(i.e., this is an exceptional edge), mirroring the temp-var suppression logic in
updateObligationsWithInvocationResult so obligations are only created for real
return value paths for MethodInvocationNode results.

3712-3718: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

cmValOfIce may be null here.

store.getValue(ice) can return null, and getCalledMethods(...) (Line 3753) dereferences cmVal.getAccumulatedValues() / cmVal.getAnnotations() without a null guard, producing an NPE on valid CFGs where the iterated-element expression has no abstract value in the called-methods store.

Proposed fix
     if (ice != null) {
       AccumulationValue cmValOfIce = store.getValue(ice);
-      List<String> calledMethods = getCalledMethods(cmValOfIce);
-      if (calledMethods != null) {
-        calledMethodsAfterThisBlock = new HashSet<>(calledMethods);
+      if (cmValOfIce != null) {
+        List<String> calledMethods = getCalledMethods(cmValOfIce);
+        if (calledMethods != null) {
+          calledMethodsAfterThisBlock = new HashSet<>(calledMethods);
+        }
       }
     }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`
around lines 3712 - 3718, store.getValue(ice) can return null so calling
getCalledMethods(cmValOfIce) may cause NPE; modify the code to null-guard the
AccumulationValue returned from store.getValue(ice) (or update getCalledMethods
to accept null) before dereferencing it: check if cmValOfIce != null and only
call getCalledMethods(cmValOfIce) then populate calledMethodsAfterThisBlock,
otherwise skip or treat as empty set; reference the variables/methods involved:
ice, store.getValue(...), AccumulationValue cmValOfIce, getCalledMethods(...),
and calledMethodsAfterThisBlock.

2129-2137: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Same null/empty list issue at the second reportError site.

Same pattern as Line 2051; mustCallValues is dereferenced via .get(0) without a null/empty guard, yielding NPE/IOOBE on legitimate inputs.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`
around lines 2129 - 2137, The reportError call in the OwningCollection case
dereferences
coAtf.getMustCallValuesOfResourceCollectionComponent(lhs.getTree()).get(0)
without null/empty checks which can cause NPE/IOOBE; assign the result to a
local variable (e.g., mustCallValues), check if it is null or empty
(mustCallValues == null || mustCallValues.isEmpty()), and pass a safe value
(null or a sensible default) to checker.reportError instead of calling .get(0)
directly; update the code paths in MustCallConsistencyAnalyzer.OwningCollection
and use the same safe-access pattern as used around line 2051.

540-540: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

CollectionObligation still missing hashCode override and immutability.

mustCallMethod is still a public, non-final field, and equals is overridden without a corresponding hashCode override. Two CollectionObligations that differ only in mustCallMethod will now have inconsistent hashCode/equals, breaking hash-based containers (and the parent Obligation is documented as deeply immutable). PMD also flags OverrideBothEqualsAndHashcode here.

Proposed fix
-    /** The method that must be called on all elements of the collection. */
-    public String mustCallMethod;
+    /** The method that must be called on all elements of the collection. */
+    public final String mustCallMethod;
@@
     `@Override`
     public boolean equals(`@Nullable` Object obj) {
       if (obj == this) {
         return true;
       }
       if (!(obj instanceof CollectionObligation collectionObligation)) {
         return false;
       }
       return super.equals(obj) && collectionObligation.mustCallMethod.equals(this.mustCallMethod);
     }
+
+    `@Override`
+    public int hashCode() {
+      return Objects.hash(super.hashCode(), mustCallMethod);
+    }
   }

Also applies to: 597-607

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`
at line 540, The CollectionObligation class exposes a public, mutable
mustCallMethod field and overrides equals without overriding hashCode; make
mustCallMethod a private final field, add a public accessor (e.g.,
getMustCallMethod()), update the constructor(s) to set it immutably, and remove
any direct writes to the field; then implement hashCode() consistent with equals
(include mustCallMethod and other equality-contributing fields) so hash-based
collections behave correctly; apply the same immutability and hashCode fix to
the other similar occurrence around the equals override at the later block.

2046-2054: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Unsafe .get(0) on a possibly-null/empty list.

getMustCallValuesOfResourceCollectionComponent(lhs.getTree()) can return null or an empty list, so the inline .get(0) may NPE or throw IndexOutOfBoundsException on real code paths. Use a fallback (e.g., UNKNOWN_METHOD_NAME) when the list is null/empty.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`
around lines 2046 - 2054, The call to
coAtf.getMustCallValuesOfResourceCollectionComponent(lhs.getTree()) may return
null or an empty list, so directly calling .get(0) can NPE or throw
IndexOutOfBounds; update the else branch in MustCallConsistencyAnalyzer (where
checker.reportError is invoked) to first retrieve the list into a local
variable, check for null or empty, and use a safe fallback (e.g.,
UNKNOWN_METHOD_NAME) when list is null/empty instead of calling .get(0), then
pass the chosen value into checker.reportError.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/EnhancedForDisposalLoopResolver.java`:
- Around line 109-162: The Javadoc and inline comments in resolveEnhancedForLoop
inaccurately claim the algorithm starts from a desugared hasNext() node and that
blockContainingHasNext refers to hasNext(); in reality
isTargetEnhancedForInvocation only accepts a next() invocation
(methodInvocationNode is next()), so update the Javadoc and the inline comments
to say the forward walk starts from the desugared iterator next() node, that
blockContainingHasNext is the block containing next(), and that the backward
walk discovers hasNext(); keep the code and variable names
(resolveEnhancedForLoop, methodInvocationNode, isTargetEnhancedForInvocation,
blockContainingHasNext) unchanged but correct the wording in the Javadoc and the
two inline comments to reflect next() as the starting point and hasNext() as the
target of the backward walk.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`:
- Around line 3688-3704: Remove unreachable commented code and personal-initial
TODOs in the loop-body handling: delete the commented `store =
cmAtf.getStoreAfterBlock(lastLoopBodyBlock);` that sits after the unconditional
throw, and replace the `TODO SCK` and commented-out `BugInCF` following the null
check for collectionElementObligation (from getObligationForVar(obligations,
disposalLoopInfo.iteratedElementTree())) with a neutral, tracked-note or remove
entirely — e.g., add a brief TODO referencing a project issue/ID that asks
"Investigate why iterated-element obligation is missing here" (no personal
initials) or remove the commented alternative behavior and leave the code
returning new HashSet<>() as the documented/sound fallback.
- Around line 3682-3693: Remove the unnecessary initializer for variable
"store": declare AccumulationStore store; (no "= null") before the conditional,
since each branch either assigns to store or throws; update the block where
lastLoopBodyBlock is checked (the conditional using BlockType.CONDITIONAL_BLOCK
and the else branch calling
cmAtf.getStoreAfter(lastLoopBodyBlock.getLastNode())) and keep the throw of new
BugInCF as-is so definite assignment is preserved.
- Around line 3623-3635: The condition checking "if (!forward.contains(update)
&& !allowedPreds.containsKey(update))" is misleading because update is
explicitly skipped from being added to forward (see the continue when
s.equals(update)), so forward.contains(update) is unreachable; change the
condition to check only allowedPreds (i.e., "if
(!allowedPreds.containsKey(update))") and remove or adjust the accompanying
comment about reachability; update references in MustCallConsistencyAnalyzer to
use the simplified check and ensure any related logic relying on
forward.contains(update) is not required elsewhere.
- Around line 954-963: The call to
coAtf.getMustCallValuesOfResourceCollectionComponent(...) can return null, so
guard uses of mustCallValues: ensure you check for null before iterating and
before calling isEmpty(); e.g., only run the for-loop and the
reportNeverEnforcedCollectionObligationIfNeeded(...) call when mustCallValues !=
null && !mustCallValues.isEmpty(). Update the block around mustCallValues,
CollectionObligation.fromTree(receiverNode.getTree(), mustCallMethod) and
reportNeverEnforcedCollectionObligationIfNeeded(node, mustCallValues.get(0),
receiverNode.getTree()) to be skipped when mustCallValues is null.

---

Duplicate comments:
In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipTransfer.java`:
- Line 324: The local variable isDiamond is computed by comparing
getTypeArguments().size() == 0; replace that check with
getTypeArguments().isEmpty() to use the preferred idiom; update the assignment
to isDiamond in CollectionOwnershipTransfer (the expression using
node.getTree().getTypeArguments()) accordingly.
- Around line 256-294: When transferOwnership is true, guard the use of argElem
returned by TreeUtils.elementFromTree: check whether argElem is null before
calling argElem.getKind(); if argElem is non-null and
argElem.getKind().isField() call checker.reportError(arg.getTree(),
"transfer.owningcollection.field.ownership", arg.getTree().toString()),
otherwise treat it as the non-field case and call replaceInStores(res, argJE,
atypeFactory.NOTOWNINGCOLLECTION); this prevents an NPE when
TreeUtils.elementFromTree returns null for literals/parenthesized/temp trees.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/EnhancedForDisposalLoopResolver.java`:
- Around line 154-156: The comparison of javax.lang.model.element.Name is using
== which can fail; in EnhancedForDisposalLoopResolver replace the identity
comparison between iteratorVariableDeclaration.getName() and
loopVariable.getName() (the assignment that sets isAssignmentOfLoopVariable)
with a content-based comparison such as
iteratorVariableDeclaration.getName().contentEquals(loopVariable.getName()) or
Objects.equals(...) so names are compared by content rather than reference.
- Around line 186-198: The match method in EnhancedForDisposalLoopResolver
currently throws BugInCF for CFG shape mismatches; change both failure branches
to return null instead so a non-matching CFG rejects the candidate rather than
aborting analysis: replace the first throw in the
blockContainingLoopCondition.getSuccessors().size() != 1 branch with a null
return, and replace the second throw in the !(maybeConditionalBlock instanceof
ConditionalBlock) branch with a null return (these references appear around
loopConditionNode, blockContainingLoopCondition, maybeConditionalBlock, and
conditionalBlock).

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`:
- Around line 1706-1745: Remove the redundant early-return guard and the dead
commented-out tempVar code inside the loopBody analysis block: delete the inner
"if (!isLoopBodyAnalysis) return;" check (the outer call already ensures
isLoopBodyAnalysis) and either remove the commented tempVar branch or replace it
with a single-line TODO noting potential temp-var handling; update the
treeForAlias assignment (currently node.getTree()) to remain as the active code
path and keep references to symbols Obligation, ResourceAlias, rhsExpr, lhsVar,
node.getTree(), typeFactory.isTempVar, and typeFactory.getTreeForTempVar in the
comment/TODO if you leave a placeholder so future reviewers can reintroduce
tempVar handling cleanly.
- Around line 909-914: The current code in MustCallConsistencyAnalyzer (inside
the logic handling OwningCollection return values) throws a BugInCF when
mustCallValues is null; instead handle the documented legitimate null return
from getMustCallValuesOfResourceCollectionComponent(...) by skipping creation of
CollectionObligation objects and returning early. Specifically, in the block
that checks mustCallValues, remove the BugInCF throw and replace it with a
no-op/early return so that when mustCallValues == null the method does not
create CollectionObligation instances and continues safely.
- Around line 2098-2106: The bug: MustCallConsistencyAnalyzer currently throws
BugInCF when lhsCoType == CollectionOwnershipType.OwningCollectionBottom even
though OwningCollectionBottom represents null and should be treated as a legal
RHS/LHS that falls through to the "first assignment / safe overwrite" handling;
fix by removing the throw for the OwningCollectionBottom branch in the method
where the check occurs (the block that currently checks lhsCoType ==
CollectionOwnershipType.OwningCollectionBottom) and let execution continue to
the existing first-assignment/safe-overwrite logic, keeping or updating the
comment to explain that OwningCollectionBottom is allowed and should not abort
analysis.
- Around line 1083-1087: The current code always calls
addObligationsForOwningCollectionReturn and
addObligationsForCreatesCollectionObligationAnno from
updateObligationsForInvocation for every outgoing edge, which fabricates
CollectionObligation entries on exceptional edges; update
updateObligationsForInvocation to consult the exceptionType parameter (or
otherwise detect an exceptional successor) and skip calling
addObligationsForOwningCollectionReturn and
addObligationsForCreatesCollectionObligationAnno when exceptionType is non-null
(i.e., this is an exceptional edge), mirroring the temp-var suppression logic in
updateObligationsWithInvocationResult so obligations are only created for real
return value paths for MethodInvocationNode results.
- Around line 3712-3718: store.getValue(ice) can return null so calling
getCalledMethods(cmValOfIce) may cause NPE; modify the code to null-guard the
AccumulationValue returned from store.getValue(ice) (or update getCalledMethods
to accept null) before dereferencing it: check if cmValOfIce != null and only
call getCalledMethods(cmValOfIce) then populate calledMethodsAfterThisBlock,
otherwise skip or treat as empty set; reference the variables/methods involved:
ice, store.getValue(...), AccumulationValue cmValOfIce, getCalledMethods(...),
and calledMethodsAfterThisBlock.
- Around line 2129-2137: The reportError call in the OwningCollection case
dereferences
coAtf.getMustCallValuesOfResourceCollectionComponent(lhs.getTree()).get(0)
without null/empty checks which can cause NPE/IOOBE; assign the result to a
local variable (e.g., mustCallValues), check if it is null or empty
(mustCallValues == null || mustCallValues.isEmpty()), and pass a safe value
(null or a sensible default) to checker.reportError instead of calling .get(0)
directly; update the code paths in MustCallConsistencyAnalyzer.OwningCollection
and use the same safe-access pattern as used around line 2051.
- Line 540: The CollectionObligation class exposes a public, mutable
mustCallMethod field and overrides equals without overriding hashCode; make
mustCallMethod a private final field, add a public accessor (e.g.,
getMustCallMethod()), update the constructor(s) to set it immutably, and remove
any direct writes to the field; then implement hashCode() consistent with equals
(include mustCallMethod and other equality-contributing fields) so hash-based
collections behave correctly; apply the same immutability and hashCode fix to
the other similar occurrence around the equals override at the later block.
- Around line 2046-2054: The call to
coAtf.getMustCallValuesOfResourceCollectionComponent(lhs.getTree()) may return
null or an empty list, so directly calling .get(0) can NPE or throw
IndexOutOfBounds; update the else branch in MustCallConsistencyAnalyzer (where
checker.reportError is invoked) to first retrieve the list into a local
variable, check for null or empty, and use a safe fallback (e.g.,
UNKNOWN_METHOD_NAME) when list is null/empty instead of calling .get(0), then
pass the chosen value into checker.reportError.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 1d6e655d-6a81-480f-a3b0-3865a2582e91

📥 Commits

Reviewing files that changed from the base of the PR and between e4a57d7 and 9a9a714.

📒 Files selected for processing (4)
  • checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipTransfer.java
  • checker/src/main/java/org/checkerframework/checker/collectionownership/EnhancedForDisposalLoopResolver.java
  • checker/src/main/java/org/checkerframework/checker/collectionownership/IndexedForDisposalLoopMatcher.java
  • checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java

Comment on lines +109 to +162
/**
* Returns resolved disposal loop info if the given node is desugared from the target
* enhanced-for-loop {@code tree} over a resource collection.
*
* <p>Starting from the desugared iterator {@code hasNext()} node, this walks forward to recover
* the loop-variable assignment and body-entry block, then walks backward to recover the loop
* condition and update block.
*
* @param methodInvocationNode the node to check
* @param tree the enhanced-for-loop being resolved
* @return the resolved loop info, or {@code null} if the node does not belong to {@code tree}
*/
private @Nullable DisposalLoopInfo resolveEnhancedForLoop(
MethodInvocationNode methodInvocationNode, @FindDistinct EnhancedForLoopTree tree) {
if (!isTargetEnhancedForInvocation(methodInvocationNode, tree)) {
return null;
}

VariableTree loopVariable = tree.getVariable();

// Walk forward from the iterator `hasNext()` node to find the assignment that initializes the
// loop variable from `next()`. The successor after that assignment is the loop-body entry
// block.
Block blockContainingHasNext = methodInvocationNode.getBlock();
if (!(blockContainingHasNext instanceof SingleSuccessorBlock singleSuccessorBlock)) {
return null;
}
Iterator<Node> nodeIterator = singleSuccessorBlock.getNodes().iterator();
Node loopVariableNode = null;
Node candidateNode;
boolean isAssignmentOfLoopVariable;
do {
while (!nodeIterator.hasNext()) {
Block successor = singleSuccessorBlock.getSuccessor();
if (!(successor instanceof SingleSuccessorBlock nextBlock)) {
return null;
}
singleSuccessorBlock = nextBlock;
nodeIterator = singleSuccessorBlock.getNodes().iterator();
}
candidateNode = nodeIterator.next();
isAssignmentOfLoopVariable = false;
if ((candidateNode instanceof AssignmentNode)
&& (candidateNode.getTree() instanceof VariableTree iteratorVariableDeclaration)) {
loopVariableNode = ((AssignmentNode) candidateNode).getTarget();
isAssignmentOfLoopVariable =
iteratorVariableDeclaration.getName() == loopVariable.getName();
}
} while (!isAssignmentOfLoopVariable);
Block loopBodyEntryBlock = singleSuccessorBlock.getSuccessor();

// Walk backward from the iterator `hasNext()` node to recover the loop-condition node and the
// loop-update/back-edge block for this desugared enhanced-for.
Block loopUpdateBlock = methodInvocationNode.getBlock();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Doc comments and blockContainingHasNext describe next(), not hasNext().

Per the established behavior of MethodInvocationNode.getEnhancedForLoop() (only attached to the next() invocation, not hasNext() — see your own clarification in the prior thread), isTargetEnhancedForInvocation only accepts methodInvocationNode when it is the next() call. Therefore:

  • The Javadoc at lines 113-115 ("Starting from the desugared iterator hasNext() node, this walks forward …") and the inline comment at 129-131 ("Walk forward from the iterator hasNext() node …") are inverted: the forward walk starts from next() and finds the loop-variable assignment.
  • blockContainingHasNext at line 132 is actually the block containing the next() invocation.
  • The inline comment at 160-161 ("Walk backward from the iterator hasNext() node …") should describe walking backward to hasNext() from the next() node.

These mislead readers about the resolver contract; the backward walk is in fact what discovers hasNext().

📝 Proposed fix
   /**
    * Returns resolved disposal loop info if the given node is desugared from the target
    * enhanced-for-loop {`@code` tree} over a resource collection.
    *
-   * <p>Starting from the desugared iterator {`@code` hasNext()} node, this walks forward to recover
-   * the loop-variable assignment and body-entry block, then walks backward to recover the loop
-   * condition and update block.
+   * <p>Starting from the desugared iterator {`@code` next()} node (the only invocation tagged with
+   * {`@link` MethodInvocationNode#getEnhancedForLoop()}), this walks forward to recover the
+   * loop-variable assignment and body-entry block, then walks backward to recover the loop
+   * condition ({`@code` hasNext()}) and update block.
    *
    * `@param` methodInvocationNode the node to check
    * `@param` tree the enhanced-for-loop being resolved
    * `@return` the resolved loop info, or {`@code` null} if the node does not belong to {`@code` tree}
    */
@@
     VariableTree loopVariable = tree.getVariable();
 
-    // Walk forward from the iterator `hasNext()` node to find the assignment that initializes the
-    // loop variable from `next()`. The successor after that assignment is the loop-body entry
-    // block.
-    Block blockContainingHasNext = methodInvocationNode.getBlock();
-    if (!(blockContainingHasNext instanceof SingleSuccessorBlock singleSuccessorBlock)) {
+    // Walk forward from the iterator `next()` node to find the assignment that initializes the
+    // loop variable from `next()`. The successor after that assignment is the loop-body entry
+    // block.
+    Block blockContainingNext = methodInvocationNode.getBlock();
+    if (!(blockContainingNext instanceof SingleSuccessorBlock singleSuccessorBlock)) {
       return null;
     }
@@
-    // Walk backward from the iterator `hasNext()` node to recover the loop-condition node and the
-    // loop-update/back-edge block for this desugared enhanced-for.
+    // Walk backward from the iterator `next()` node to recover the loop-condition (`hasNext()`)
+    // node and the loop-update/back-edge block for this desugared enhanced-for.
     Block loopUpdateBlock = methodInvocationNode.getBlock();
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/EnhancedForDisposalLoopResolver.java`
around lines 109 - 162, The Javadoc and inline comments in
resolveEnhancedForLoop inaccurately claim the algorithm starts from a desugared
hasNext() node and that blockContainingHasNext refers to hasNext(); in reality
isTargetEnhancedForInvocation only accepts a next() invocation
(methodInvocationNode is next()), so update the Javadoc and the inline comments
to say the forward walk starts from the desugared iterator next() node, that
blockContainingHasNext is the block containing next(), and that the backward
walk discovers hasNext(); keep the code and variable names
(resolveEnhancedForLoop, methodInvocationNode, isTargetEnhancedForInvocation,
blockContainingHasNext) unchanged but correct the wording in the Javadoc and the
two inline comments to reflect next() as the starting point and hasNext() as the
target of the backward walk.

Comment on lines +954 to +963
List<String> mustCallValues =
coAtf.getMustCallValuesOfResourceCollectionComponent(receiverNode.getTree());
for (String mustCallMethod : mustCallValues) {
obligations.add(
CollectionObligation.fromTree(receiverNode.getTree(), mustCallMethod));
}
if (!mustCallValues.isEmpty()) {
reportNeverEnforcedCollectionObligationIfNeeded(
node, mustCallValues.get(0), receiverNode.getTree());
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

mustCallValues may be null; null-check needed before iteration.

Per learnings (and past reviews), coAtf.getMustCallValuesOfResourceCollectionComponent(...) can legitimately return null for owning collections over non-resource element types. Iterating it (Line 956) and calling isEmpty() (Line 960) without a null guard will NPE for those programs.

Proposed fix
           if (!receiverIsOwningField) {
             List<String> mustCallValues =
                 coAtf.getMustCallValuesOfResourceCollectionComponent(receiverNode.getTree());
+            if (mustCallValues == null) {
+              return;
+            }
             for (String mustCallMethod : mustCallValues) {
               obligations.add(
                   CollectionObligation.fromTree(receiverNode.getTree(), mustCallMethod));
             }
             if (!mustCallValues.isEmpty()) {
               reportNeverEnforcedCollectionObligationIfNeeded(
                   node, mustCallValues.get(0), receiverNode.getTree());
             }
           }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`
around lines 954 - 963, The call to
coAtf.getMustCallValuesOfResourceCollectionComponent(...) can return null, so
guard uses of mustCallValues: ensure you check for null before iterating and
before calling isEmpty(); e.g., only run the for-loop and the
reportNeverEnforcedCollectionObligationIfNeeded(...) call when mustCallValues !=
null && !mustCallValues.isEmpty(). Update the block around mustCallValues,
CollectionObligation.fromTree(receiverNode.getTree(), mustCallMethod) and
reportNeverEnforcedCollectionObligationIfNeeded(node, mustCallValues.get(0),
receiverNode.getTree()) to be skipped when mustCallValues is null.

Comment on lines +3623 to +3635
if (s.equals(update)) {
continue;
}
if (forward.add(s)) {
wl.addLast(s);
}
}
}
// If update isn't even reachable from entry under the filtered edge relation,
// region is empty.
if (!forward.contains(update) && !allowedPreds.containsKey(update)) {
return Collections.emptySet();
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial | 💤 Low value

!forward.contains(update) is unreachable; the check simplifies to !allowedPreds.containsKey(update).

update is explicitly skipped from being added to forward by the continue at Line 3624, so forward.contains(update) can never be true. The combined && reads as if both could be false simultaneously, which is misleading.

Proposed simplification
-    if (!forward.contains(update) && !allowedPreds.containsKey(update)) {
+    if (!allowedPreds.containsKey(update)) {
       return Collections.emptySet();
     }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`
around lines 3623 - 3635, The condition checking "if (!forward.contains(update)
&& !allowedPreds.containsKey(update))" is misleading because update is
explicitly skipped from being added to forward (see the continue when
s.equals(update)), so forward.contains(update) is unreachable; change the
condition to check only allowedPreds (i.e., "if
(!allowedPreds.containsKey(update))") and remove or adjust the accompanying
comment about reachability; update references in MustCallConsistencyAnalyzer to
use the simplified check and ensure any related logic relying on
forward.contains(update) is not required elsewhere.

Comment on lines +3682 to +3693
AccumulationStore store = null;
if (lastLoopBodyBlock.getType() == BlockType.CONDITIONAL_BLOCK) {
ConditionalBlock conditionalBlock = (ConditionalBlock) lastLoopBodyBlock;
@SuppressWarnings("interning:not.interned")
boolean thenSuccessor = conditionalBlock.getThenSuccessor() == loopUpdateBlock;
store = cmAtf.getStoreAfterConditionalBlock(conditionalBlock, thenSuccessor);
} else if (lastLoopBodyBlock.getLastNode() == null) {
throw new BugInCF("Loop Body Analysis -- Block " + lastLoopBodyBlock + " has no nodes");
// store = cmAtf.getStoreAfterBlock(lastLoopBodyBlock);
} else {
store = cmAtf.getStoreAfter(lastLoopBodyBlock.getLastNode());
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial | 💤 Low value

Drop the unused store = null initializer (per PMD UnusedAssignment).

All branches either assign store or throw, so the initializer is dead. Declaring store without an initializer also lets the compiler verify definite assignment if a branch is added later.

Proposed fix
-    AccumulationStore store = null;
+    AccumulationStore store;
     if (lastLoopBodyBlock.getType() == BlockType.CONDITIONAL_BLOCK) {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
AccumulationStore store = null;
if (lastLoopBodyBlock.getType() == BlockType.CONDITIONAL_BLOCK) {
ConditionalBlock conditionalBlock = (ConditionalBlock) lastLoopBodyBlock;
@SuppressWarnings("interning:not.interned")
boolean thenSuccessor = conditionalBlock.getThenSuccessor() == loopUpdateBlock;
store = cmAtf.getStoreAfterConditionalBlock(conditionalBlock, thenSuccessor);
} else if (lastLoopBodyBlock.getLastNode() == null) {
throw new BugInCF("Loop Body Analysis -- Block " + lastLoopBodyBlock + " has no nodes");
// store = cmAtf.getStoreAfterBlock(lastLoopBodyBlock);
} else {
store = cmAtf.getStoreAfter(lastLoopBodyBlock.getLastNode());
}
AccumulationStore store;
if (lastLoopBodyBlock.getType() == BlockType.CONDITIONAL_BLOCK) {
ConditionalBlock conditionalBlock = (ConditionalBlock) lastLoopBodyBlock;
`@SuppressWarnings`("interning:not.interned")
boolean thenSuccessor = conditionalBlock.getThenSuccessor() == loopUpdateBlock;
store = cmAtf.getStoreAfterConditionalBlock(conditionalBlock, thenSuccessor);
} else if (lastLoopBodyBlock.getLastNode() == null) {
throw new BugInCF("Loop Body Analysis -- Block " + lastLoopBodyBlock + " has no nodes");
// store = cmAtf.getStoreAfterBlock(lastLoopBodyBlock);
} else {
store = cmAtf.getStoreAfter(lastLoopBodyBlock.getLastNode());
}
🧰 Tools
🪛 PMD (7.24.0)

[Medium] 3682-3682: The initializer for variable 'store' is never used (overwritten on lines 3687 and 3692) (UnusedAssignment)

(Best Practices)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`
around lines 3682 - 3693, Remove the unnecessary initializer for variable
"store": declare AccumulationStore store; (no "= null") before the conditional,
since each branch either assigns to store or throws; update the block where
lastLoopBodyBlock is checked (the conditional using BlockType.CONDITIONAL_BLOCK
and the else branch calling
cmAtf.getStoreAfter(lastLoopBodyBlock.getLastNode())) and keep the throw of new
BugInCF as-is so definite assignment is preserved.

Comment on lines +3688 to +3704
} else if (lastLoopBodyBlock.getLastNode() == null) {
throw new BugInCF("Loop Body Analysis -- Block " + lastLoopBodyBlock + " has no nodes");
// store = cmAtf.getStoreAfterBlock(lastLoopBodyBlock);
} else {
store = cmAtf.getStoreAfter(lastLoopBodyBlock.getLastNode());
}
Obligation collectionElementObligation =
getObligationForVar(obligations, disposalLoopInfo.iteratedElementTree());
if (collectionElementObligation == null) {
// the loop did something weird. Might have reassigned the collection element.
// The sound thing to do is return an empty list.
// TODO SCK look at this. Why is the collection element obligaiton gone?
return new HashSet<>();
// throw new BugInCF(
// "No obligation for collection element "
// + potentiallyFulfillingLoop.collectionElementTree);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial | 💤 Low value

Clean up stale commented code and personal-initial TODO.

  • Line 3690 has a commented-out store = cmAtf.getStoreAfterBlock(lastLoopBodyBlock); immediately after an unconditional throw — it’s unreachable and confusing.
  • Lines 3699-3704 contain a TODO SCK with author initials and another commented-out BugInCF. Personal-initial TODOs and dead commented branches should be replaced with a tracked TODO that captures the actual question (why the iterated-element obligation is missing here) or removed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`
around lines 3688 - 3704, Remove unreachable commented code and personal-initial
TODOs in the loop-body handling: delete the commented `store =
cmAtf.getStoreAfterBlock(lastLoopBodyBlock);` that sits after the unconditional
throw, and replace the `TODO SCK` and commented-out `BugInCF` following the null
check for collectionElementObligation (from getObligationForVar(obligations,
disposalLoopInfo.iteratedElementTree())) with a neutral, tracked-note or remove
entirely — e.g., add a brief TODO referencing a project issue/ID that asks
"Investigate why iterated-element obligation is missing here" (no personal
initials) or remove the commented alternative behavior and leave the code
returning new HashSet<>() as the documented/sound fallback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants